-
Notifications
You must be signed in to change notification settings - Fork 84
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
feat: introduce experimental feature switch #613
Conversation
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #613 +/- ##
=======================================
Coverage 34.43% 34.43%
=======================================
Files 32 32
Lines 1844 1844
=======================================
Hits 635 635
Misses 1188 1188
Partials 21 21 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ab76830
to
a90cf97
Compare
321757c
to
1c31401
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.
One comment on sub-commands.
728aaf1
to
5c2738d
Compare
@priteshbandi I have removed policy changes, can you help review the implementation @yizha1 @FeynmanZhou @iamsamirzon @toddysm can you help review the user experience as well as the wording for the output in the PR description, thanks! |
2866e1f
to
5dd2527
Compare
825c394
to
e1bb96f
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
|
||
// IsDisabled determines whether an experimental feature is disabled. | ||
func IsDisabled() bool { | ||
return os.Getenv(envName) != enabled |
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.
should we trim the white spaces here, for both key and value?
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.
For what purpose do we need to trim key and value?
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.
In scenario where user calls following command to set env variable.
export NOTATION_EXPERIMENTAL=" 1"
Its an edge case, I am fine to ignore this.
|
||
// Error returns an error for a disabled experimental feature. | ||
func Error(description string) error { | ||
return fmt.Errorf("%s been marked as experimental and not enabled by default. To use it, please set %s=%s in your environment", description, envName, enabled) |
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.
return fmt.Errorf("%s been marked as experimental and not enabled by default. To use it, please set %s=%s in your environment", description, envName, enabled) | |
return fmt.Errorf("%s is experimental and not enabled by default. To use, please set %s=%s environment variable", description, envName, enabled) |
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.
Changed accordingly. There is a case when multiple experimental flags the warning can be
flag(s) --foo,--bar,--baz in "notation test" is experimental and not enabled by default. To use, please set %s=%s environment variable
I am okay with the is
here, @patrickzheng200 what do you think.
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.
Changed accordingly. There is a case when multiple experimental flags the warning can be
flag(s) --foo,--bar,--baz in "notation test" is experimental and not enabled by default. To use, please set %s=%s environment variable
I am okay with the
is
here, @patrickzheng200 what do you think.
Yeah, we could have multiple flags marked as experimental, in this case, using is
here is not very accurate.
Signed-off-by: Billy Zha <jinzha1@microsoft.com> clean Signed-off-by: Billy Zha <jinzha1@microsoft.com> hide experimental Signed-off-by: Billy Zha <jinzha1@microsoft.com> fix e2e Signed-off-by: Billy Zha <jinzha1@microsoft.com> code clean Signed-off-by: Billy Zha <jinzha1@microsoft.com> remove policy changes Signed-off-by: Billy Zha <jinzha1@microsoft.com> update flag checker Signed-off-by: Billy Zha <jinzha1@microsoft.com> update type Signed-off-by: Billy Zha <jinzha1@microsoft.com> make output more readable Signed-off-by: Billy Zha <jinzha1@microsoft.com> resolve comments Signed-off-by: Billy Zha <jinzha1@microsoft.com> resolve comments Signed-off-by: Billy Zha <jinzha1@microsoft.com> remove unnecessary change Signed-off-by: Billy Zha <jinzha1@microsoft.com> resolve comments Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
Signed-off-by: Billy Zha <jinzha1@microsoft.com>
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
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
This PR introduces an environmental variable,
NOTATION_EXPERIMENTAL
, as the switch for experimental featuresNOTATION_EXPERIMENTAL
set to1
, then experimental features are enabled. All experimental features can be used with a prompt in the stderr output:Caution: This feature is experimental and may not be fully tested or completed and may be deprecated. Report any issues to "https://github/notaryproject/notation"
Error: "<COMMAND>" been marked as experimental and not enabled by default. To use it, please set NOTATION_EXPERIMENTAL=1 in your environment
or
Error: flags(s) --<FLAG1>,--<FLAG2> in "<COMMAND>" been marked as experimental and not enabled by default. To use it, please set NOTATION_EXPERIMENTAL=1 in your environment
Resolves #614.