command: print appropriate warning messages on 'context list'/'contex… by nicks · Pull Request #3668 · docker/cli (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation14 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Codecov Report
Merging #3668 (f31397f) into master (c59773f) will increase coverage by
0.02%.
The diff coverage is80.00%.
❗ Current head f31397f differs from pull request most recent head 49f4db9. Consider uploading reports for the commit 49f4db9 to get more accurate results
@@ Coverage Diff @@ ## master #3668 +/- ##
Coverage 59.01% 59.03% +0.02%
Files 289 289
Lines 24623 24626 +3- Hits 14532 14539 +7
- Misses 9218 9215 -3
- Partials 873 872 -1
| if os.Getenv(client.EnvOverrideHost) != "" { |
|---|
| if _, present := os.LookupEnv(client.EnvOverrideHost); present { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't looked closely at the other places where this is checked, but wondering if instead of checking if the env-var is found, we should just take the "simple" approach and treat "empty" and "unset" as equivalent everywhere.
When scripting, it's not uncommon to have a variable set to an empty string (to "reset it"), and I think it would be ok for us to ignore in that case.
@thaJeztah I updated the PR so that it treats DOCKER_HOST="" and DOCKER_HOST unset the same way.
| @@ -456,7 +456,7 @@ func resolveContextName(opts *cliflags.CommonOptions, config *configfile.ConfigF |
|---|
| if len(opts.Hosts) > 0 { |
| return DefaultContextName, nil |
| } |
| if _, present := os.LookupEnv(client.EnvOverrideHost); present { |
| if os.Getenv(client.EnvOverrideHost) != "" { |
| return DefaultContextName, nil |
| } |
| if ctxName, ok := os.LookupEnv("DOCKER_CONTEXT"); ok { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also do the same thing with DOCKER_CONTEXT, for consistency?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, missed your comment; yes, I think we should do the same (Or at least, I don't see a good reason to consider an empty env-var to be used 🤔)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated!
Comment on lines 46 to 48
| host := os.Getenv(client.EnvOverrideHost) |
|---|
| isIneffective := host != "" && configValue != command.DefaultContextName |
| if isIneffective { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: need to have another look at this logic 🤔
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Looks like this one needs to check either configValue != "" or name != command.DefaultContextName, because a couple of lines up, configValue is set to an empty string when using default;
| configValue := name |
|---|
| if configValue == "default" { |
| configValue = "" |
| } |
print appropriate warning messages on 'context list'/'context use'
Signed-off-by: Nick Santos nick.santos@docker.com Signed-off-by: Sebastiaan van Stijn github@gone.nl
Signed-off-by: Nick Santos nick.santos@docker.com
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks; I found some minor issues, and one issue with the check; we can probably also squash the first two commits (as the second one is touching-up the first one).
I was writing along while reviewing, so let me push to your branch to get this going.
Comment on lines 46 to 48
| host := os.Getenv(client.EnvOverrideHost) |
|---|
| isIneffective := host != "" && configValue != command.DefaultContextName |
| if isIneffective { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Looks like this one needs to check either configValue != "" or name != command.DefaultContextName, because a couple of lines up, configValue is set to an empty string when using default;
| configValue := name |
|---|
| if configValue == "default" { |
| configValue = "" |
| } |
Comment on lines 65 to 78
| assert.Assert(t, |
|---|
| strings.Contains( |
| cli.ErrBuffer().String(), |
| `Warning: DOCKER_HOST environment variable overrides the active context.`)) |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to the above; we could probably add a test here then that verifies that the warning is not triggered for default.
| err = newUseCommand(cli).RunE(nil, []string{"test"}) |
|---|
| assert.NilError(t, err) |
| assert.Assert(t, !strings.Contains(out.String(), "Warning")) |
| assert.Assert(t, strings.Contains(out.String(), "Current context is now \"test\"")) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit; could probably use ` as quotes make it slightly more readable
For the string comparing, we can use gotest.tools/v3/assert/cmp, which prints a nice diff if things aren't equal.
| configDir := t.TempDir() |
|---|
| config.SetDir(configDir) |
| socketPath := fmt.Sprintf("unix://%s", filepath.Join(configDir, "docker.sock")) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit; for simple cases, we generally prefer to just concatenate;
socketPath := "unix://" + filepath.Join(configDir, "docker.sock")
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is happy; let's get this one in!
LGTM
