Add env variable option for experimental#1138
Conversation
cli/command/cli.go
Outdated
|
|
||
| func isEnabled(value string) (bool, error) { | ||
| // Environment variable always overrides configuration | ||
| if os.Getenv("DOCKER_CLI_EXPERIMENTAL") != "" { |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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
|
|
||
| func isEnabled(value string) (bool, error) { | ||
| // Environment variable always overrides configuration | ||
| if os.Getenv("DOCKER_CLI_EXPERIMENTAL") != "" { |
There was a problem hiding this comment.
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 |
silvin-lubecki
left a comment
There was a problem hiding this comment.
Thank you @seemethere , 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)
