Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

context: implement lazy loading, and other improvements #3847

Merged
merged 7 commits into from
Nov 28, 2022

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Nov 4, 2022

cli/command: load context, and initialize client lazily

This allows commands that don't require a client connection (such as context use)
to be functional, but still produces an error when trying to run a command that
needs to connect with the API;

mkdir -p ~/.docker/ && echo '{"currentContext":"nosuchcontext"}' >  ~/.docker/config.json
docker version
Failed to initialize: unable to resolve docker endpoint: load context "nosuchcontext": context does not exist: open /root/.docker/contexts/meta/8bfef2a74c7d06add4bf4c73b0af97d9f79c76fe151ae0e18b9d7e57104c149b/meta.json: no such file or directory

docker context use default
default
Current context is now "default"

docker version
Client:
 Version:           22.06.0-dev
 API version:       1.42
 ...

cli/command/context: context ls: add ERROR column, and don't fail early

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.

cli/command/context: context ls: always show current context

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…

@thaJeztah thaJeztah added this to the 22.06.0 milestone Nov 4, 2022
@thaJeztah thaJeztah marked this pull request as draft November 4, 2022 15:27
@thaJeztah thaJeztah force-pushed the context_lazy_evaluate branch from 77c9c9b to b8fd21b Compare November 4, 2022 15:43
cli/command/context/show.go Outdated Show resolved Hide resolved
cli/command/cli.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Nov 21, 2022

Codecov Report

Merging #3847 (de52868) into master (81c6891) will decrease coverage by 0.00%.
The diff coverage is n/a.

❗ Current head de52868 differs from pull request most recent head 740dde0. Consider uploading reports for the commit 740dde0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3847      +/-   ##
==========================================
- Coverage   59.22%   59.22%   -0.01%     
==========================================
  Files         287      287              
  Lines       24656    24662       +6     
==========================================
+ Hits        14603    14606       +3     
- Misses       9171     9172       +1     
- Partials      882      884       +2     

@thaJeztah thaJeztah force-pushed the context_lazy_evaluate branch 2 times, most recently from f297b87 to 740dde0 Compare November 28, 2022 12:37
@thaJeztah thaJeztah force-pushed the context_lazy_evaluate branch from 740dde0 to 8c49008 Compare November 28, 2022 15:09
This allows commands that don't require a client connection (such as `context use`)
to be functional, but still produces an error when trying to run a command that
needs to connect with the API;

    mkdir -p ~/.docker/ && echo '{"currentContext":"nosuchcontext"}' >  ~/.docker/config.json
    docker version
    Failed to initialize: unable to resolve docker endpoint: load context "nosuchcontext": context does not exist: open /root/.docker/contexts/meta/8bfef2a74c7d06add4bf4c73b0af97d9f79c76fe151ae0e18b9d7e57104c149b/meta.json: no such file or directory

    docker context use default
    default
    Current context is now "default"

    docker version
    Client:
     Version:           22.06.0-dev
     API version:       1.42
     ...

Signed-off-by: Sebastiaan van Stijn <[email protected]>
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 <[email protected]>
@thaJeztah thaJeztah force-pushed the context_lazy_evaluate branch from 8c49008 to 868f1fc Compare November 28, 2022 15:53
@thaJeztah thaJeztah marked this pull request as ready for review November 28, 2022 17:10
@thaJeztah
Copy link
Member Author

Okay; moved this one out of draft; @vvoland @silvin-lubecki PTAL

@@ -0,0 +1,3 @@
NAME DESCRIPTION DOCKER ENDPOINT ERROR
default Current DOCKER_HOST based configuration unix:///var/run/docker.sock
nosuchcontext * load context "nosuchcontext": context does n…
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first reaction was that it feels weird to have list output a non-existing context. On the second thought it presents the real state more explicitly, so I think it's okay.

One thing that could be better IMO is the visibility of the "context does not exist error", which seems to be hidden because of the ellipsis.
Could we shorten the error to "context does not exist" in this case? The name is already in the NAME column, so I think that load context "nosuchcontext" doesn't add much value here.
This could still make the "context does not exist" invisible, if the name is long though.

We could also the default order of the columns to: NAME, ENDPOINT, ERROR, DESCRIPTON.
Name and endpoint seem to be the most important bits, error in most cases should be empty so shouldn't take up much space, and truncating description is probably the least harmful.

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we shorten the error to "context does not exist" in this case? The name is already in the NAME column, so I think that load context "nosuchcontext" doesn't add much value here.
This could still make the "context does not exist" invisible, if the name is long though.

Yes, I was looking at that originally; the tricky bit here was that the error is produced in a part of the code that's called in quite some places, and I couldn't immediately judge if removing the name from the error would hide relevant information (which context failed to load?) in all of those cases. We can definitely look at this again, and perhaps improve. I also want to add the --no-trunc option (have some of that stashed locally), but will keep that for a follow-up.

I tweaked things a bit; changedc the load context "name of context" to context "name of context", and changed does not exist to not found;

NAME              DESCRIPTION                               DOCKER ENDPOINT               ERROR
default           Current DOCKER_HOST based configuration   unix:///var/run/docker.sock   
nosuchcontext *                                                                           context "nosuchcontext": context not found: …

We could also the default order of the columns to: NAME, ENDPOINT, ERROR, DESCRIPTION.
Name and endpoint seem to be the most important bits, error in most cases should be empty so shouldn't take up much space, and truncating description is probably the least harmful.

I stuck with current conventions of putting the ERROR as last column, but also agree that I always was a bit on the fence for DESCRIPTION altogether; perhaps NAME, ENDPOINT, DESCRIPTION, ERROR? Let's fine-tune this in a follow up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the tricky bit here was that the error is produced in a part of the code that's called in quite some places

Oh, actually; we can change it; I can check if it's an errdefs.IsErrNotFound(), and in that case replace it with context not found.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, but if we do, then we're hiding the underlying error as well.

Alright; let's look at improving in a follow-up.

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 *                                                                           context "nosuchcontext": context not found: …

Signed-off-by: Sebastiaan van Stijn <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants