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

Add docker context item #231

Closed
wants to merge 7 commits into from
Closed

Conversation

Crocmagnon
Copy link
Contributor

@Crocmagnon Crocmagnon commented Dec 5, 2021

Description

Add an item displaying the current docker context. Largely inspired by the kubectl item.

Screenshots (if appropriate)

Screenshot 2021-12-05 at 20 36 24

How Has This Been Tested

  • I have tested using Linux.
  • I have tested using MacOS.

Checklist

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@IlanCosman
Copy link
Owner

Thanks! I'll take a look. Don't worry about mega-linter.

@IlanCosman
Copy link
Owner

Can you give me a quick run-down on how you tested this? I have everything set up, but I'm not very knowledgeable about docker.

@Crocmagnon
Copy link
Contributor Author

Crocmagnon commented Dec 5, 2021

Sure! First, thank you for your quick feedback 😃

With docker installed, you can list contexts using docket context ls. There should be at least one named default. If you use the default context, no output will be shown as with the kubectl item.
You can create a new context with another name and switch to using it, the item should then update.

Here’s Docker’s documentation about contexts including how to create and switch to a new context: https://docs.docker.com/engine/context/working-with-contexts/

Also, I didn’t know how to find a glyph so I picked one using this tool, hope this works.

@IlanCosman
Copy link
Owner

Found a problem. docker context show isn't available with normal docker, it requires docker-compose. You can switch this item to using docker context ls or docker context inspect at your discretion.

The latter is not available on all systems.
@Crocmagnon
Copy link
Contributor Author

Crocmagnon commented Dec 6, 2021

Done 👍🏻

Side note: funnily enough, docker context show is twice as fast as docker inspect + format. (58 vs 100ms)

@IlanCosman
Copy link
Owner

Thanks, looks pretty good 👍 I'll get back to this in a few days, I need to study for finals 😄

@IlanCosman
Copy link
Owner

I'm a bit concerned about performance 😞 Even with async, I'm not super excited about 20-40 ms extra (on my machines).

kubectl has garbage performance, but I'm more ok with that because fewer people use it.

@Crocmagnon
Copy link
Contributor Author

Crocmagnon commented Dec 10, 2021

Well, considering that docker contexts are kind of a power user feature, and power users are very likely to also have docker compose, and IIRC, compose is now bundled into docker (it’s now available as a subcommand docker compose instead of a separate docker-compose) ; would it be ok to revert to the previous better performing version of the item (with context show)?

(how were your finals? 😊)

@IlanCosman
Copy link
Owner

IlanCosman commented Dec 10, 2021

Well, considering that docker contexts are kind of a power user feature, and power users are very likely to also have docker compose, and IIRC, compose is now bundled into docker (it’s now available as a subcommand docker compose instead of a separate docker-compose) ; would it be ok to revert to the previous better performing version of the item (with context show)?

Just tried it out. docker compose is only available after you've installed docker-compose. So not really bundled together.
And I haven't gotten docker context show to work at all, even with docker compose installed.

"docker context show" on google gives very few results, and no official documentation.


I think that we should follow what spacefish and starship are doing here, which is only running the item if some files exist.
From Starship:

detect_files ["docker-compose.yml", "docker-compose.yaml", "Dockerfile"] Which filenames should trigger this module (needs only_with_files to be true).

Could you add that? Don't make it configurable though.

(how were your finals? 😊)

They went ok 😄

@Crocmagnon
Copy link
Contributor Author

Yeah, I couldn't find any official documentation on docker context show, but I found it in the github issue template on the official compose-cli repository though (https://github.com/docker/compose-cli/issues/new).

Just for the record, I'm currently using version 20.10.11 of Docker Desktop for Mac. The same version on linux doesn't seem to offer docker context show indeed.

Anyway, I tried with docker context ls | grep "*" | cut -f 1 -d '*' | xargs but it takes about the same time as the previous version (inspect + format).

I don't think limiting this to a repository containing some arbitrary files would be useful. As with k8s contexts, this affects docker commands globally.

@IlanCosman
Copy link
Owner

I don't think limiting this to a repository containing some arbitrary files would be useful. As with k8s contexts, this affects docker commands globally.

Yes, it effects docker globally, but the question is when are you going to need to see it.

The optimal solution AFAICT would be #90, where the docker context only shows up if your command line has docker in it.

We generally use the files as a proxy for that though. How often are you going to be running docker commands when there are no docker related files nearby? (I don't know the answer btw, I don't use docker 😄)

@Crocmagnon
Copy link
Contributor Author

Crocmagnon commented Dec 10, 2021

How often are you going to be running docker commands when there are no docker related files nearby?

That's a very interesting question. Since I mostly use docker compose to run stuff with docker on my machine, I'd say that in this case I would nearly always have a compose or dockerfile nearby.
However, having recently discovered docker contexts, my use case is slightly different: I'm managing a remote server with docker installed and I want to access its docker daemon easily from my machine, to view logs for example. In this case, I don't think I'll have a compose file nearby, and that's when viewing the context would matter the most.

Is the detect_files function already implemented?

@IlanCosman
Copy link
Owner

Is the detect_files function already implemented?

Yes, it's called test 😄

In this case, I don't think I'll have a compose file nearby, and that's when viewing the context would matter the most.

Fair enough. Then let's not detect files, and just have this item not be on by default 👍 (so no changes necessary)

@IlanCosman
Copy link
Owner

IlanCosman commented Dec 10, 2021

Merged with some changes into the fish-3.4 branch 👍 Thanks for contributing 😄

EDIT: 7224a99, gave credit :)

@IlanCosman IlanCosman closed this Dec 10, 2021
@IlanCosman
Copy link
Owner

Oh, dang, it didn't give you credit for any of your commits. I'll try to fix that.

IlanCosman added a commit that referenced this pull request Dec 10, 2021
Co-authored-by: Gabriel Augendre <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants