-
Notifications
You must be signed in to change notification settings - Fork 14
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
Update feature flags apis #140
Update feature flags apis #140
Conversation
d8dee28
to
50ea7f1
Compare
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.
LGTM. Just one minor suggestion on API name/signature.
50ea7f1
to
fa5fa0b
Compare
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.
LGTM. Thanks
fa5fa0b
to
38f4900
Compare
@@ -10,19 +10,23 @@ import ( | |||
var ( | |||
// legacyLocalDirName is the name of the old local directory in which to look for tanzu state. This will be | |||
// removed in the future in favor of LocalDirName. | |||
// Deprecated: This legacyLocalDirName is deprecated. Use next get config apis |
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.
All the new deprecations in this file are on private functions. Is there a reason we can’t just remove them?
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.
We already have our legacy store marked for deprecation I have just added these related private methods as well to be consistent. We would remove these when we remove the legacy store after the deprecation window.
} | ||
|
||
// IsFeatureFlagExists returns true if the features section in the configuration object contains any value for the plugin.feature combination | ||
func (c *ClientConfig) IsFeatureFlagExists(plugin, feature string) bool { |
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.
That function name doesn’t sound good in English. How about just FeatureFlagExists? Or IsFeatureFlagSet?
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.
Updated the method name
38f4900
to
e54184b
Compare
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.
LGTM
Thanks
What this PR does / why we need it
- GetAllFeatureFlags
- ConfigureDefaultFeatureFlags
- IsConfigFeatureActivated
- IsFeatureFlagExists
- GetAllFeatureFlags
Which issue(s) this PR fixes
Fixes #
Describe testing done for PR
Release note
Additional information
Special notes for your reviewer