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

Add boolean validation for tanzu config set feature flags #685

Merged

Conversation

mpanchajanya
Copy link
Contributor

@mpanchajanya mpanchajanya commented Feb 27, 2024

What this PR does / why we need it

  • Add validation for tanzu config set feature flag value to restrict to boolean true or false values.

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

  • Added unit tests
  • Manual testing
> tz config get | grep foo
> tz config set features.global.foo bar
[x] : invalid value provided only boolean true or false are accepted
> tz config set features.global.foo true
> tz config set features.global.foo false
> tz config set features.global.foo bar
[x] : invalid value provided only boolean true or false are accepted
> tz config set features.global.foo ""
[x] : invalid value provided only boolean true or false are accepted
> tz config set features.global.foo true
> tz config get | grep foo
            foo: "true"
> 

Release note

Tanzu CLI will only allow boolean true or false feature flag values

Additional information

Special notes for your reviewer

@mpanchajanya mpanchajanya marked this pull request as ready for review February 28, 2024 15:53
@mpanchajanya mpanchajanya requested a review from a team as a code owner February 28, 2024 15:53
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.

For completeness let's mention that this PR is technically not a backwards-compatible change. A script could currently run tz config set features.cluster.myfeat TRUE and everything would work but now it will not be able to. Considering the scenarios, I think a small breaking change is ok but I'd like opinions.
@mpanchajanya @vuil @anujc25

If we want to reduce the risk of breakage we could allow the widest, valid and consistent values. Those values must work for both APIs, so they would be true/false, TRUE/FALSE and True/False. That being said, I think true/false is enough but I'd also like opinions.

@@ -145,6 +145,9 @@ func setConfiguration(pathParam, value string) error {
if len(paramArray) != 3 {
return errors.New("unable to parse config path parameter into three parts [" + strings.Join(paramArray, ".") + "] (was expecting 'features.<plugin>.<feature>'")
}
if value != "true" && value != "false" { //nolint:goconst
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as information:

To properly choose the values we should allow, I thought of checking how the values could be read.
I believe there are two APIs to get a feature flag.

The first API:

config.IsFeatureEnabled("plugin", "flag")

This one uses strings.EqualFold(val, "true") so it accepts case-insensitive true and everything else is false
Ref: https://github.com/vmware-tanzu/tanzu-plugin-runtime/blob/6362240e00b2c0b891141c4d75fe525714e1c755/config/features.go#L40

The other API:

cfg, _ := config.GetClientConfig()
cfg.IsConfigFeatureActivated("features.plugin.flag")

This one uses strconv.ParseBool() which accepts 1, t, T, TRUE, true, True, 0, f, F, FALSE, false, False.
Ref: https://github.com/vmware-tanzu/tanzu-plugin-runtime/blob/6362240e00b2c0b891141c4d75fe525714e1c755/config/types/clientconfig_features.go#L40

This PR can help fix this slight inconsistency by only allowing values that work consistently for both APIs.
What you have coded does that 👍

@marckhouzam marckhouzam added this to the v1.3.0 milestone Mar 12, 2024
@anujc25
Copy link
Contributor

anujc25 commented Mar 13, 2024

For completeness let's mention that this PR is technically not a backwards-compatible change. A script could currently run tz config set features.cluster.myfeat TRUE and everything would work but now it will not be able to. Considering the scenarios, I think a small breaking change is ok but I'd like opinions. @mpanchajanya @vuil @anujc25

If we want to reduce the risk of breakage we could allow the widest, valid and consistent values. Those values must work for both APIs, so they would be true/false, TRUE/FALSE and True/False. That being said, I think true/false is enough but I'd also like opinions.

Thanks for the explanation. Yes, I think this should be fine to accept as the field was used to set a boolean so far.

@mpanchajanya mpanchajanya merged commit 5cb6f89 into vmware-tanzu:main Mar 18, 2024
7 checks passed
vuil pushed a commit to vuil/tanzu-cli that referenced this pull request Mar 28, 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.

None yet

4 participants