-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 env variable option for experimental #1138
Conversation
cli/command/cli.go
Outdated
@@ -180,6 +180,10 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error { | |||
} | |||
|
|||
func isEnabled(value string) (bool, error) { | |||
// Environment variable always overrides configuration | |||
if os.Getenv("DOCKER_CLI_EXPERIMENTAL") != "" { |
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.
Can you update the documentation, and add this env-var to the list in https://github.com/docker/cli/blob/master/docs/reference/commandline/cli.md#environment-variables ?
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.
So this means that DOCKER_CLI_EXPERIMENTAL=false docker manifest
will be valid? Maybe we should match the configuration file values instead of just checking the existence of the environment variable?
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.
This does not seem very consistent at the moment, for instance there is
func contentTrustEnabled() bool {
if e := os.Getenv("DOCKER_CONTENT_TRUST"); e != "" {
if t, err := strconv.ParseBool(e); t || err != nil {
// treat any other value as true
return true
}
}
return false
}
but
func hide(cmd *cobra.Command) *cobra.Command {
// If the environment variable with name "DOCKER_HIDE_LEGACY_COMMANDS" is not empty,
// these legacy commands (such as `docker ps`, `docker exec`, etc)
// will not be shown in output console.
if os.Getenv("DOCKER_HIDE_LEGACY_COMMANDS") == "" {
return cmd
}
…
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.
Oh right...
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.
Yeah for it to be uniform with what the value in the config is, it would need to be either enabled
or disabled
. Should I change it to that?
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.
Here's what the diff would look like for that:
diff --git a/cli/command/cli.go b/cli/command/cli.go
index 37e3e10e..0597341d 100644
--- a/cli/command/cli.go
+++ b/cli/command/cli.go
@@ -182,7 +182,7 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error {
func isEnabled(value string) (bool, error) {
// Environment variable always overrides configuration
if os.Getenv("DOCKER_CLI_EXPERIMENTAL") != "" {
- return true, nil
+ value = os.Getenv("DOCKER_CLI_EXPERIMENTAL")
}
switch value {
case "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.
Thank you for this PR @seemethere ! At first we wanted the user to explicitly enable the experimental features only by adding a value in the config file. But maybe with env var it is also explicit enough? WDYT @vdemeester ?
cli/command/cli.go
Outdated
@@ -180,6 +180,10 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error { | |||
} | |||
|
|||
func isEnabled(value string) (bool, error) { | |||
// Environment variable always overrides configuration | |||
if os.Getenv("DOCKER_CLI_EXPERIMENTAL") != "" { |
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.
So this means that DOCKER_CLI_EXPERIMENTAL=false docker manifest
will be valid? Maybe we should match the configuration file values instead of just checking the existence of the environment variable?
7326275
to
5c88f6f
Compare
5c88f6f
to
b86a3a9
Compare
Signed-off-by: Eli Uriegas <eli.uriegas@docker.com>
b86a3a9
to
e3bb62e
Compare
Updated to environment variable to only accept |
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.
Thank you @seemethere , 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 🐸
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
Signed-off-by: Eli Uriegas eli.uriegas@docker.com
- What I did
Added an option to enable the experimental cli options with an environment variable
- How to verify it
Proof of work
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)