context: improvements for handling non-existing contexts and errors #3868

thaJeztah · 2022-11-17T13:11:52Z

This is a subset of context: implement lazy loading, and other improvements #3847, without the lazy loading part
relates to / addresses Difficult to execute commands when currentContext is invalid #1955
relates to / addresses [BUG] Docker using context is unusable if it cannot connect to daemon - can't change context, print version, build image #3096

codecov-commenter · 2022-11-17T13:33:21Z

Codecov Report

Merging #3868 (685449f) into master (2080390) will decrease coverage by 0.00%.
The diff coverage is 45.76%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3868      +/-   ##
==========================================
- Coverage   59.23%   59.23%   -0.01%     
==========================================
  Files         287      287              
  Lines       24656    24691      +35     
==========================================
+ Hits        14605    14625      +20     
- Misses       9168     9183      +15     
  Partials      883      883

thaJeztah · 2022-11-17T13:52:23Z

cli/command/context/show.go

 func runShow(dockerCli command.Cli) error {
 	context := dockerCli.CurrentContext()
-	metadata, err := dockerCli.ContextStore().GetMetadata(context)
+	fmt.Fprintln(dockerCli.Out(), context)
+	_, err := dockerCli.ContextStore().GetMetadata(context)
 	if err != nil {
 		return err
 	}
-	fmt.Fprintln(dockerCli.Out(), metadata.Name)
 	return nil
 }


As this is a new command, we can still change; do we think we should produce an error at all if the context wasn't found, or just "show the name of the context" (basically: this is what's configured to use, but you'll learn if it works once you use it)?

We could still opt for printing the error (on STDERR) as a warning, but make the command always return a zero exit code ("success").

It seems like it should mimic existing behavior for other commands. e.g. for inspect:

if dockerCli.CurrentContext() == "" { return errors.New("no context specified") }

Hm... right; so docker context inspect expects a positional argument. It looks like there's a bug in the compose-cli wrapper, as it hijacks the docker context subcommands, and it looks like the usage information is incorrect;

This is the output for compose-cli;

docker context inspect --help Display detailed information on one or more contexts Usage: docker context inspect [flags] Flags: -f, --format string Format the output using the given Go template -h, --help Help for inspect

And the output for the actual docker cli;

com.docker.cli context inspect --help Usage: docker context inspect [OPTIONS] [CONTEXT] [CONTEXT...] Display detailed information on one or more contexts Options: -f, --format string Format the output using the given Go template

As to the docker context show; that command was added in #3567. Basically its sole purpose is to show what context is configured, so that it can be used in prompts. An earlier PR (#2500) added it to docker info for that purpose, but docker context show is more convenient.

As it (likely) will be used for prompts, the question is, should it always validate? Without validation, the value is still valid (it shows what's configured), but even if we check if the context is valid (can be found, and decoded), it may still be pointing to a host that's not reachable.

sam-thibault · 2022-11-21T12:54:17Z

cli/command/context/testdata/list.golden

-default     Current DOCKER_HOST based configuration   unix:///var/run/docker.sock
-other       description of other                      https://someswarmserver.example.com
-unset       description of unset                      https://someswarmserver.example.com
+NAME        DESCRIPTION                               DOCKER ENDPOINT                       ERROR


ERROR looks like a copy/paste mistake here

It's the title for the ERROR column that was added, but looking at this, it looks like I have more work to do. The context store itself already fails "full stop" if it encounters an error, so without some of the other changes from #3847, it looks like this doesn't work 😞

thaJeztah · 2022-11-21T16:31:52Z

Hm... more bugs (or at least differences in behaviour) in compose cli; looks like it's handling contexts differently, and happily goes ahead with empty contexts 🤔

docker context ls
NAME                TYPE                DESCRIPTION                               DOCKER ENDPOINT                                   KUBERNETES ENDPOINT   ORCHESTRATOR
                    moby
default             moby                Current DOCKER_HOST based configuration   unix:///var/run/docker.sock                                             swarm
desktop-linux *     moby                                                          unix:///Users/thajeztah/.docker/run/docker.sock

com.docker.cli context ls
cannot find docker endpoint in context

thaJeztah · 2022-11-21T16:38:14Z

And ... buildx does it different as well;

make shell
# build dockerfile from stdin so that we don't send the build-context; source is bind-mounted in the development environment
cat ./dockerfiles/Dockerfile.dev | docker build  --build-arg=GO_VERSION -t docker-cli-dev -
ERROR: use `docker --context= buildx` to switch to context
make[1]: *** [build_docker_image] Error 1
make: *** [dev] Error 2

Looks to be related to context store handling / ignoring name, which then causes it to use "" as context name, instead of the default; https://github.com/docker/buildx/blob/6e9b743296d8ba029871516370909c6f809dd935/commands/util.go#L201-L225

Store a reference to the options in the client, so that they can be used for later use. Now that resolveContextName() is a very lightweight function we can also dynamically get its value and use it for cli.CurrentContext(). Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This internalizes constructing the Client(), which allows us to provide fallbacks when trying to determin the current API version. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This updates `docker context ls` to: - not abort listing contexts when failing one (or more) contexts - instead, adding an ERROR column to inform the user there was an issue loading the context. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

if a context is set (e.g. through DOCKER_CONTEXT or the CLI config file), but wasn't found, then a "stub" context is added, including an error message that the context doesn't exist. DOCKER_CONTEXT=nosuchcontext docker context ls NAME DESCRIPTION DOCKER ENDPOINT ERROR default Current DOCKER_HOST based configuration unix:///var/run/docker.sock nosuchcontext * load context "nosuchcontext": context does n… Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This allows the user to still see what context is _set_ in cases where it does not exist; docker context show nosuchcontext load context "nosuchcontext": context does not exist: open /root/.docker/contexts/meta/8bfef2a74c7d06add4bf4c73b0af97d9f79c76fe151ae0e18b9d7e57104c149b/meta.json: no such file or directory The error is printed on stderr, so can be redirected: docker context show 2> /dev/null nosuchcontext echo $? 1 This can help in cases where the context is used to (e.g.) set the command-prompt, but the user removed the context. With this change, the context name can still be shown, but commands that _require_ the context will still fail. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2022-12-05T16:34:03Z

Closing as all of these were merged

thaJeztah added impact/changelog status/2-code-review impact/documentation area/context labels Nov 17, 2022

thaJeztah added this to the 22.06.0 milestone Nov 17, 2022

thaJeztah force-pushed the context_improvements branch from e358130 to 005b180 Compare November 17, 2022 13:29

thaJeztah marked this pull request as ready for review November 17, 2022 13:48

thaJeztah commented Nov 17, 2022

View reviewed changes

thaJeztah force-pushed the context_improvements branch from 005b180 to 01fe846 Compare November 17, 2022 23:23

sam-thibault reviewed Nov 21, 2022

View reviewed changes

thaJeztah mentioned this pull request Nov 22, 2022

The docker context list command's output with formatting is inconsistent on Linux/Windows/Mac moby/moby#44469

Open

thaJeztah added 5 commits November 23, 2022 00:26

cli/command: add Cli.CurrentVersion() function

37b6dce

This internalizes constructing the Client(), which allows us to provide fallbacks when trying to determin the current API version. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the context_improvements branch from 01fe846 to 685449f Compare November 22, 2022 23:27

thaJeztah marked this pull request as draft November 22, 2022 23:27

thaJeztah mentioned this pull request Nov 28, 2022

cli/command/context: "docker context show": don't validate context #3886

Merged

thaJeztah closed this Dec 5, 2022

thaJeztah deleted the context_improvements branch December 5, 2022 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

context: improvements for handling non-existing contexts and errors #3868

context: improvements for handling non-existing contexts and errors #3868

Uh oh!

thaJeztah commented Nov 17, 2022 •

edited

Loading

Uh oh!

codecov-commenter commented Nov 17, 2022 •

edited

Loading

Uh oh!

thaJeztah Nov 17, 2022 •

edited

Loading

Uh oh!

sam-thibault Nov 21, 2022

Uh oh!

thaJeztah Nov 21, 2022

Uh oh!

thaJeztah Nov 21, 2022

Uh oh!

sam-thibault Nov 21, 2022

Uh oh!

thaJeztah Nov 21, 2022

Uh oh!

thaJeztah commented Nov 21, 2022

Uh oh!

thaJeztah commented Nov 21, 2022

Uh oh!

thaJeztah commented Dec 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

context: improvements for handling non-existing contexts and errors #3868

context: improvements for handling non-existing contexts and errors #3868

Uh oh!

Conversation

thaJeztah commented Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

thaJeztah Nov 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-thibault Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

sam-thibault Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Nov 21, 2022

Uh oh!

thaJeztah commented Nov 21, 2022

Uh oh!

thaJeztah commented Dec 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thaJeztah commented Nov 17, 2022 •

edited

Loading

codecov-commenter commented Nov 17, 2022 •

edited

Loading

thaJeztah Nov 17, 2022 •

edited

Loading