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 bash completion for new docker image command family #27719

Merged
merged 12 commits into from
Oct 26, 2016

Conversation

albers
Copy link
Member

@albers albers commented Oct 25, 2016

This is the second part of adding bash completion for #26025 (#27579 being the first one): docker image *.

Like in #27579, I created one commit for the pricipal flow of logic and subsequent ones that move the completion logic to the new homes. This was done so that the changes can be easily reviewed (as all the changes were applied to the same file, the overall diff is not very helpful).

One of the commits is special. From the commit message:

In #23614 docker inspect was semantically enhanced to inspect "everything".
Therefore moving its logic to _docker_container_inspect in #27579 was not correct.

This commit moves it back to its original top-level location (_docker_inspect) so that it can be called by _docker_{container,image}_inspect and others (will be added in follow-up PRs).
Parameterization was added in order to get caller-specific behavior.

For the new command docker image prune I just added a stub. I'll implement it in a follow-up PR.

ping @thaJeztah @mlaventure @tianon

Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
…rameterized function

In moby#23614 `docker inspect` was semantically enhanced to inspect "everything".
Therefore moving its logic to `_docker_container_inspect` was not correct.

This commit moves it back to its original top-level location (`_docker_inspect`)
so that it can be called by `_docker_{container,image}_inspect` and others (will
be added in follow-up PRs).
Parameterization was added in order to get caller-specific behavior.

Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
@mlaventure
Copy link
Contributor

Testing this atm and I've stumbled into the following issue:

# docker  image ls
REPOSITORY          TAG                 IMAGE ID            CREATED             SIZE
ubuntu              trusty              1e0c3dd64ccd        11 days ago         188 MB
busybox             latest              e02e811dd08f        2 weeks ago         1.09 MB
debian              wheezy              517034637339        5 weeks ago         84.9 MB
# docker image history <TAB>
busybox         busybox:latest  debian          debian:wheezy   ubuntu          ubuntu:trusty   
# docker image history 

ubuntu and debian shouldn't be part of the output since I don't have ubuntu:latest and debian:latest in my image list.

@albers
Copy link
Member Author

albers commented Oct 26, 2016

@mlaventure I did not change behavior here, and I also think it's correct, see:

Usage:  docker images [OPTIONS] [REPOSITORY[:TAG]]

ubuntu is not short for ubuntu:latest. It is the repository name and stands for all images in the ubuntu repository, think of ubuntu:*.
The completion is a combined list of repository names and tagged images.

@mlaventure
Copy link
Contributor

@albers in my example ubuntu:latest doesn't exist, so if i accept this completion the command fails.

It can be changed in a later PR, but I'd say the current behavior is incorrect as it causes the command to fail.

@albers
Copy link
Member Author

albers commented Oct 26, 2016

@mlaventure Sorry, I didn't read your post properly, thought the problem was in docker images.

I agree that the completion is wrong. docker image history expects an image as its argument. Completion lists repositories and tagged images, like in docker images.

As this is an existing bug, I suggest to fix it in a separate PR.
I will file a bug for this.

@mlaventure
Copy link
Contributor

LGTM in that case, all my other tests passed.

Thanks @albers !

Copy link
Member

@tianon tianon left a comment

Choose a reason for hiding this comment

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

❤️ 😍 LGTM

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants