-
Notifications
You must be signed in to change notification settings - Fork 4
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
Feature Flags #508
base: main
Are you sure you want to change the base?
Feature Flags #508
Conversation
Signed-off-by: Sarah McClure <[email protected]>
Signed-off-by: Sarah McClure <[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.
I have a couple of questions, but looks good to me.
client := paragliderpb.NewCloudPluginClient(conn) | ||
cloudFlags := ¶gliderpb.PluginFlags{KubernetesClustersEnabled: cfg.FeatureFlags.PluginFlags[c.Name].KubernetesClustersEnabled, | ||
PrivateEndpointsEnabled: cfg.FeatureFlags.PluginFlags[c.Name].PrivateEndpointsEnabled} | ||
_, err = client.SetFlags(context.Background(), ¶gliderpb.SetFlagsRequest{Flags: cloudFlags}) |
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.
missing logging or returning err?
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.
Good catch!
pkg/orchestrator/config/config.go
Outdated
AttachResourceEnabled bool `yaml:"attachResourceEnabled"` | ||
} | ||
|
||
type PluginFlags struct { |
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.
Did you consider having a flat struct containing all feature flags? I think I like the simplicity of it, plus it doesn't prevent use from having a feature flag that both orchestrator and plugins need to reason about.
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.
I did, and this may be a good change to make. However, I went with this direction since it makes it extremely clear which ones the plugin developer is responsible for. Otherwise, they may implement some logic for a flag that will only ever be used in the orchestrator. Happy to discuss more though.
Signed-off-by: Sarah McClure <[email protected]>
Signed-off-by: Sarah McClure <[email protected]>
Signed-off-by: Sarah McClure <[email protected]>
Signed-off-by: Sarah McClure <[email protected]>
TODO:
Resolves #506