Decouple Feature-Flags from cli/runtime library and move to it to plugin specific code #3665
Conversation
|
Cluster Generation A/B Results: |
anujc25
force-pushed
the
feature-flag-dep-remove-v3
branch
from
eea7447 to
5b7d66d
Compare
|
Cluster Generation A/B Results: |
anujc25
force-pushed
the
feature-flag-dep-remove-v3
branch
2 times, most recently
from
1aecc07 to
ffdfa37
Compare
|
Cluster Generation A/B Results: |
|
Cluster Generation A/B Results: |
There was a problem hiding this comment.
I haven't looked at the code yet, but I noticed that I still see the tkr-version-v1alpha3-beta feature flag under the global list. Is this correct?
$ rm ~/.config/tanzu/config.yaml
$ tanzu config init
✔ successfully initialized the config
$ tanzu config get |grep global: -A5
global:
context-aware-cli-for-plugins: "true"
context-target: "false"
tkr-version-v1alpha3-beta: "false"
management-cluster:
aws-instance-types-exclude-arm: "true"
The constant used for this option is |
There was a problem hiding this comment.
This is a vey nice decoupling. Some minor comments in line and one question below.
When we delete a plugin, we don't delete its feature flags from the config file. That is probably on purpose so that if the user installs the plugin back, they keep their configuration? I think this is a good approach, I just wanted to confirm in case the question comes up later.
| FeatureFlagPackageBasedLCM = "features.global.package-based-lcm-beta" | ||
| // TKR version v1alpha3 feature flag determines whether to use Tanzu Kubernetes Release API version v1alpha3. Setting | ||
| // feature flag to true will allow to use the TKR version v1alpha3; false allows to use legacy TKR version v1alpha1 | ||
| FeatureFlagTKRVersionV1Alpha3 = "features.global.tkr-version-v1alpha3-beta" |
There was a problem hiding this comment.
Was it a mistake that this constant uses "features.global"? I guess we cannot correct it because it would break existing user config files?
If that is the case, it would deserve a comment in the new location of this constant
There was a problem hiding this comment.
Yeah. I believe this should have been used as a plugin specific constants but it was used as global assuming multiple plugin might rely on them.
Considering, these feature-flags are always false by default at the moment, we might be able to change it if required. But that means all testing scripts need to be updated as well.
There was a problem hiding this comment.
What testing scripts are you referring to? I don't see that string used in TF anywhere else.
There was a problem hiding this comment.
I was more talking about product level testing and not in TF testing.
There was a problem hiding this comment.
Also, we should update the config-feature.md file with the new approach.
tanzu-framework/cli/core/docs/cli/config-features.md
Lines 34 to 50 in a6760d3
Codecov Report
@@ Coverage Diff @@
## main #3665 +/- ##
==========================================
- Coverage 46.60% 45.62% -0.99%
==========================================
Files 400 425 +25
Lines 39726 41277 +1551
==========================================
+ Hits 18516 18832 +316
- Misses 19513 20731 +1218
- Partials 1697 1714 +17
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
anujc25
force-pushed
the
feature-flag-dep-remove-v3
branch
from
ffdfa37 to
60b1d67
Compare
|
Cluster Generation A/B Results: |
| descriptors, err := cli.ListPlugins() | ||
|
|
||
| for _, desc := range append(serverPluginDescriptors, standalonePluginDescriptors...) { | ||
| config.AddDefaultFeatureFlagsIfMissing(cfg, desc.DefaultFeatureFlags) |
There was a problem hiding this comment.
What happens when the core CLI is managing older plugins that do not provide these default flags?
There was a problem hiding this comment.
Older plugins can take care of updating this DefaultFeatureFlag by themselves because of the way they are importing the tanzu-framework as a dependency. There is a func init that gets run every-time which configures this feature-flags if they are missing with each plugin invocation.
| // TODO: cli.ListPlugins is deprecated: Use pluginmanager.AvailablePluginsFromLocalSource or pluginmanager.AvailablePlugins instead | ||
| descriptors, err := cli.ListPlugins() | ||
|
|
||
| for _, desc := range append(serverPluginDescriptors, standalonePluginDescriptors...) { |
There was a problem hiding this comment.
The (newly added) context-specific aspect of config init is not very intuitive (especially for folks who have used it before without having to think about this aspect). To properly reconstruct all the defaults in the config file, one may now have to switch contexts so that all plugins which offers default values are covered. If the user does not learn of this new behavior, he could think that a single config init would fix/initialize the config file, without realizing that it only partially initializes the file.
Would iterating through all installed plugins be an option?
If we choose to stick with the current implementation, I would still suggest
- putting a comment to note this behavior
- see if we can provide some doc and/or UX feedback to the user about the same (maybe in the help text of the config init command)
There was a problem hiding this comment.
I think iterating through all installed plugins might be overkill. Considering user might have substantial amount of plugins installed and some of them are not used anymore.
I think adding a more details in the tanzu config init help text and providing comment in the code should be sufficient I feel. I will do the necessary changes to add more details.
There was a problem hiding this comment.
I think that sounds reasonable. We should make sure any docs we produce capture the details as well.
I suggest we tag the issue and this PR with docs-triage-needed to raise some awareness.
Signed-off-by: Anuj Chaudhari <anujc@vmware.com>
Signed-off-by: Anuj Chaudhari <anujc@vmware.com>
…ig init` Signed-off-by: Anuj Chaudhari <anujc@vmware.com>
Signed-off-by: Anuj Chaudhari <anujc@vmware.com>
anujc25
force-pushed
the
feature-flag-dep-remove-v3
branch
from
60b1d67 to
1e6d87d
Compare
Signed-off-by: Anuj Chaudhari <anujc@vmware.com>
anujc25
force-pushed
the
feature-flag-dep-remove-v3
branch
from
1e6d87d to
468a334
Compare
|
Cluster Generation A/B Results: |
|
Cluster Generation A/B Results: |
vuil
added
ok-to-merge
What this PR does / why we need it
cli/runtimelibrary and move it to plugin specific codePluginDescriptorfor a specific plugin to set the defaults for the feature-flags and core CLI configures the default values during plugin installation or during plugin upgrade.tanzu config initcommand if config file is updated manually and defaults needs to be set again.Which issue(s) this PR fixes
Fixes #3447
Describe testing done for PR
Manual tests:
tanzu config getcommandtanzu plugin syncto install all pluginstanzu config getand verify plugin specific default feature-flags are configured correctly.tanzu config initUnit tests are written and validated as part of CI
Release note
Additional information
Special notes for your reviewer