-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Improve and fix bash completion for images #717
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #717 +/- ##
==========================================
- Coverage 53.51% 53.29% -0.22%
==========================================
Files 218 218
Lines 14565 14565
==========================================
- Hits 7795 7763 -32
- Misses 6289 6326 +37
+ Partials 481 476 -5 |
8cd7a85 to
783d7f0
Compare
| fi | ||
| shift | ||
| ;; | ||
| --force-tag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it should be additive with --tag, like --tag --force or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work because it is not clear what should be forced. My intent is to use --repo, --tag and --id to express that a representation can be completed (if permitted by environment settings) and to use --force-repo, --force-tag and --force-id when a representation has to be added, regardless of environment settings. Of the latter, currently only --force-tag is used, so the other options from this set are not yet implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also want to keep this function very simple. That's why it doesn't care about the order of the arguments. Thus, a --force is not deterministic. And I want the invocations to fit on one line, so I opted for the short names like --tag instead of something more exact like --try-tag or --allow-tag.
I think this simplicity is appropriate for this function because it is only used internally in the bash completion script.
contrib/completion/bash/docker
Outdated
| __docker_q image ls --no-trunc --format "${format%\\n}" $all "$@" | grep -v '<none>$' | ||
| } | ||
|
|
||
| # __docker_complete_images appies completion of images based on the current value of `$cur` or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: typo "appies" should be "applies"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, I'll change that. Thanks.
783d7f0 to
1f86efe
Compare
Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
1f86efe to
53376b6
Compare
vdemeester
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cc @mlaventure
mlaventure
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
whoops forgot about this one LGTM, thanks! |
|
@thaJeztah Would you mind looking at #682? It's been hanging around even longer. |
Improve and fix bash completion for images Upstream-commit: 4586609 Component: cli
Shortcomings of the current solution
Bash completion of images uses the vanilla output of
docker imagesand filters it withawk.This is error prone because users may customize the output of
docker imagesby setting a go template in ~/.docker/config.json with theimagesFormatkey.And it is ugly.
There are several valid combinations of repository, tag and ID in Docker CLI. This diversity is not sufficiently covered yet, leading to issues like moby/moby#27791.
Furthermore, problems arise because completion of images is customizable through environment variables:
DOCKER_COMPLETION_SHOW_TAGS=no.DOCKER_COMPLETION_SHOW_IMAGE_IDS=none(default)|non-intermediate|allThese settings are not fine-grained enough, though.
docker history, we have to show tags even if the user opted to hide them.Solution
docker imageslike in the completions of containers, networks, volumes... .--repo,--tag,--id) to one central completion function and let this function decide what to complete wrt environment settings.--force-tag)We now use the following combinations:
__docker_complete_images --force-tag --id__docker_complete_images --repo__docker_complete_images --repo --tag__docker_complete_images --repo --tag --idI prefer to leave the invocations as listed here instead of creating 4 helper functions because these would not increase readibility. The explicit list of legal choices very clearly expresses differences between related completetions, see
images --filter (before|since)=andimages --filter reference=for an example.Fixes
For these cases,
--repois not used and--force-tagsis applied. Only valid tags are completed.image ls repoandimage pull --all-tags repofailed for repository names containing a:(e.g. localhost:5000/some-repo)Changes
Completion of
image importnow completes local files instead of nothing. The previous choice was made because URLs are also valid syntax. The result feels like a broken completion.Some minor changes in style were applied in a separate commit in order to increase code consistency.
Testing
If you have bash completion installed and working, just source the updated completion script.
The main helper function can also be called directly to study the influence of the customizion via environment variables, e.g.
ping @mlaventure Fixes moby/moby#27791, PTAL