command: print appropriate warning messages on 'context list'/'contex… by nicks · Pull Request #3668 · docker/cli

nicks · 2022-06-07T21:59:41Z

- What I did
print appropriate warning messages on 'docker context list' and 'docker context use'

- How I did it
Makes sure that the use/list implementations use the same logic for checking env
variables as the code that processes the override.

- How to verify it
see attached issues

fixes docker context use fails silently when DOCKER_HOST set to empty string #3667
fixes Confusing warning when selecting default context with DOCKER_HOST set #1932

- Description for the changelog
print appropriate warning messages on 'docker context list' and 'docker context use'

- A picture of a cute animal (not mandatory but encouraged)

codecov-commenter · 2022-06-07T22:02:36Z

Codecov Report

Merging #3668 (f31397f) into master (c59773f) will increase coverage by 0.02%.
The diff coverage is 80.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

thaJeztah · 2022-06-08T20:31:31Z

cli/command/context/list.go

 		return err
 	}
-	if os.Getenv(client.EnvOverrideHost) != "" {
+	if _, present := os.LookupEnv(client.EnvOverrideHost); present {


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.

nicks · 2022-06-09T16:50:40Z

@thaJeztah I updated the PR so that it treats DOCKER_HOST="" and DOCKER_HOST unset the same way.

nicks · 2022-06-09T16:53:28Z

cli/command/cli.go

+	if os.Getenv(client.EnvOverrideHost) != "" {
 		return DefaultContextName, nil
 	}
 	if ctxName, ok := os.LookupEnv("DOCKER_CONTEXT"); ok {


should we also do the same thing with DOCKER_CONTEXT, for consistency?

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 🤔)

thaJeztah · 2022-07-05T08:59:27Z

cli/command/context/use.go

+	host := os.Getenv(client.EnvOverrideHost)
+	isIneffective := host != "" && configValue != command.DefaultContextName
+	if isIneffective {


Note to self: need to have another look at this logic 🤔

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;

cli/cli/command/context/use.go

Lines 34 to 37 in a445d97

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>

thaJeztah

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.

thaJeztah · 2022-07-29T08:11:03Z

cli/command/context/use.go

+	host := os.Getenv(client.EnvOverrideHost)
+	isIneffective := host != "" && configValue != command.DefaultContextName
+	if isIneffective {


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;

cli/cli/command/context/use.go

Lines 34 to 37 in a445d97

configValue := name

if configValue == "default" {

configValue = ""

}

thaJeztah · 2022-07-29T08:12:10Z

cli/command/context/use_test.go

+	assert.Assert(t,
+		strings.Contains(
+			cli.ErrBuffer().String(),
+			`Warning: DOCKER_HOST environment variable overrides the active context.`))
+}


Related to the above; we could probably add a test here then that verifies that the warning is not triggered for default.

thaJeztah · 2022-07-29T08:13:26Z

cli/command/context/use_test.go

+	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\""))


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.

thaJeztah · 2022-07-29T08:16:13Z

cli/command/context/use_test.go

+	configDir := t.TempDir()
+	config.SetDir(configDir)
+
+	socketPath := fmt.Sprintf("unix://%s", filepath.Join(configDir, "docker.sock"))


nit; for simple cases, we generally prefer to just concatenate;

socketPath := "unix://" + filepath.Join(configDir, "docker.sock")

thaJeztah

CI is happy; let's get this one in!

LGTM

nicks · 2022-07-29T20:56:35Z

woooo, thanks!!

thaJeztah reviewed Jun 8, 2022

View reviewed changes

nicks force-pushed the nicks/issue-3667 branch 3 times, most recently from 49f4db9 to 2e494c1 Compare June 9, 2022 16:50

nicks force-pushed the nicks/issue-3667 branch 2 times, most recently from b622717 to c5d33b9 Compare June 9, 2022 16:53

nicks commented Jun 9, 2022

View reviewed changes

nicks requested a review from thaJeztah June 9, 2022 17:18

thaJeztah added the status/2-code-review label Jun 30, 2022

thaJeztah added this to the 22.06.0 milestone Jul 5, 2022

thaJeztah reviewed Jul 5, 2022

View reviewed changes

nicks force-pushed the nicks/issue-3667 branch from 70fccc5 to dd6198e Compare July 14, 2022 18:23

nicks requested a review from thaJeztah July 14, 2022 18:23

nicks added 2 commits July 29, 2022 11:04

command: treat DOCKER_HOST the same if it's empty or unset

aa7b1b2

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>

also treat DOCKER_CONTEXT='' as unset

50893d7

Signed-off-by: Nick Santos <nick.santos@docker.com>

thaJeztah mentioned this pull request Jul 29, 2022

context use: don't create/update config file and directories if not needed #3721

Merged

thaJeztah requested changes Jul 29, 2022

View reviewed changes

thaJeztah force-pushed the nicks/issue-3667 branch from dd6198e to 50893d7 Compare July 29, 2022 10:09

thaJeztah added impact/changelog kind/bugfix PR's that fix bugs labels Jul 29, 2022

thaJeztah approved these changes Jul 29, 2022

View reviewed changes

thaJeztah merged commit 7963778 into docker:master Jul 29, 2022

nicks deleted the nicks/issue-3667 branch July 29, 2022 20:56

bzhb mentioned this pull request Aug 31, 2022

docker context use fails silently when DOCKER_HOST is set #3765

Closed

	configValue := name
	if configValue == "default" {
	configValue = ""
	}

Conversation

nicks commented Jun 7, 2022 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicks commented Jun 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

nicks commented Jul 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

nicks commented Jun 7, 2022 •

edited by thaJeztah

Loading

codecov-commenter commented Jun 7, 2022 •

edited

Loading