-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test(flagd): adding configuration gherkin tests #651
base: main
Are you sure you want to change the base?
test(flagd): adding configuration gherkin tests #651
Conversation
Signed-off-by: Raphael Wigoutschnigg <[email protected]>
Signed-off-by: Raphael Wigoutschnigg <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, this is the foundation for our gherkin tests and it is really really cool. I am not sure if we should opt in for reflection and not utilize the helper methods, what do you think?
providers/flagd/pkg/provider.go
Outdated
@@ -235,147 +195,147 @@ func (p *Provider) setStatus(status of.State) { | |||
|
|||
// ProviderOptions | |||
|
|||
type ProviderOption func(*Provider) | |||
type ProviderOption func(*ProviderConfiguration) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should those methods also be part of the configuration.go? as they are really configuration related?
} | ||
|
||
// providerOptionGeneratorMap a map that defines the provider options generators for supported options | ||
var providerOptionGeneratorMap = map[string]func(value string) (flagd.ProviderOption, error){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need this mapping? we can access the properties of the providerConfiguration directly - the helper methods are nice and simulate a more go like way, but i think we can run this test with directly setting the properties, wdyt? we even could set those fields via reflection
Signed-off-by: Raphael Wigoutschnigg <[email protected]>
This PR
adding configuration gherkin tests