Skip to content
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

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

mpanchajanya
Copy link
Contributor

@mpanchajanya mpanchajanya commented Dec 15, 2023

What this PR does / why we need it

  • Added New Feature APIs
    - GetAllFeatureFlags
    - ConfigureDefaultFeatureFlags
  • Added new ClientConfig methods
    - IsConfigFeatureActivated
    - IsFeatureFlagExists
    - GetAllFeatureFlags
  • Deprecate legacy methods and variables

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

  • Added new tests for the new APIs

Release note


Additional information

Special notes for your reviewer

@mpanchajanya mpanchajanya self-assigned this Dec 15, 2023
@mpanchajanya mpanchajanya force-pushed the update_features_apis branch 3 times, most recently from d8dee28 to 50ea7f1 Compare January 3, 2024 15:37
@mpanchajanya mpanchajanya marked this pull request as ready for review January 3, 2024 15:44
@mpanchajanya mpanchajanya requested a review from a team as a code owner January 3, 2024 15:44
Copy link
Contributor

@anujc25 anujc25 left a 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.

config/features.go Outdated Show resolved Hide resolved
@mpanchajanya mpanchajanya requested a review from anujc25 January 8, 2024 11:02
Copy link
Contributor

@anujc25 anujc25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks

@@ -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
Copy link
Contributor

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?

Copy link
Contributor Author

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the method name

Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks

@mpanchajanya mpanchajanya merged commit fb8415e into vmware-tanzu:main Jan 10, 2024
4 checks passed
@marckhouzam marckhouzam added this to the v1.2.0 milestone Jan 16, 2024
vuil pushed a commit to vuil/tanzu-plugin-runtime that referenced this pull request Jan 22, 2024
vuil pushed a commit that referenced this pull request Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants