command: print appropriate warning messages on 'context list'/'contex… #3668

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

-	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

@@ -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 {


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 <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

Signed-off-by: Nick Santos <[email protected]>

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 <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

also treat DOCKER_CONTEXT='' as unset

50893d7

Signed-off-by: Nick Santos <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

command: print appropriate warning messages on 'context list'/'contex… #3668

command: print appropriate warning messages on 'context list'/'contex… #3668

nicks commented Jun 7, 2022 •

edited by thaJeztah

Loading

codecov-commenter commented Jun 7, 2022 •

edited

Loading

thaJeztah Jun 8, 2022

nicks commented Jun 9, 2022

nicks Jun 9, 2022

thaJeztah Jul 5, 2022

nicks Jul 14, 2022

thaJeztah Jul 5, 2022

thaJeztah Jul 29, 2022

thaJeztah left a comment

thaJeztah Jul 29, 2022

thaJeztah Jul 29, 2022

thaJeztah Jul 29, 2022

thaJeztah Jul 29, 2022

thaJeztah left a comment

nicks commented Jul 29, 2022

		if os.Getenv(client.EnvOverrideHost) != "" {
		if _, present := os.LookupEnv(client.EnvOverrideHost); present {

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

command: print appropriate warning messages on 'context list'/'contex… #3668

command: print appropriate warning messages on 'context list'/'contex… #3668

Conversation

nicks commented Jun 7, 2022 • edited by thaJeztah Loading

codecov-commenter commented Jun 7, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

nicks commented Jun 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

nicks commented Jul 29, 2022

nicks commented Jun 7, 2022 •

edited by thaJeztah

Loading

codecov-commenter commented Jun 7, 2022 •

edited

Loading