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 experimental CLI commands (manifest) #1252

Merged
merged 3 commits into from
Nov 29, 2018

Conversation

albers
Copy link
Collaborator

@albers albers commented Aug 1, 2018

This adds bash completion for docker manifest and its subcommands.

Completion for the manifest command is only activated if experimental CLI features are enabled (either by setting "experimental": "enabled" in .docker/config.json or the env var DOCKER_CLI_EXPERIMENTAL=enabled). This configuration setting is harvested from the output of docker version.

Before adding the third configuration setting that is extracted from docker version's output , I refactored the existing logic to a more consistent and clearer state. There is only one call to docker version. It is executed lazily and caches its result.

Note: The initial concept also included gathering the orchestrator in this step, but this piece of information went bananas in #1137.

In the context of docker version and its output, I changed the domain language to use server instead of daemon because this is the term used there (bounded context). daemon is better suited for local calls whereas server is more appropriate for (potentially) remote calls.

Edit: The list of valid os/arch values was taken from validOSArches in util.go. A comment was left as a reminder to keep both files in sync.

@codecov-io
Copy link

codecov-io commented Aug 1, 2018

Codecov Report

Merging #1252 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1252   +/-   ##
=======================================
  Coverage   54.73%   54.73%           
=======================================
  Files         292      292           
  Lines       19267    19267           
=======================================
  Hits        10545    10545           
  Misses       8063     8063           
  Partials      659      659

Copy link
Collaborator

@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.

SGTM 🎍

@albers
Copy link
Collaborator Author

albers commented Aug 1, 2018

@vdemeester It's still very basic with regard of the arguments. I don't know what are valid values for MANIFEST_LIST and MANIFEST:

Usage:  docker manifest create MANIFEST_LIST MANIFEST [MANIFEST...]

So I do not complete anything right now, allowing any string.

@albers
Copy link
Collaborator Author

albers commented Aug 1, 2018

@vdemeester Do you know who I might ask for help?

@vdemeester
Copy link
Collaborator

cc @clnperez @estesp @thaJeztah 👼

@albers albers force-pushed the completion-cli-experimental branch 2 times, most recently from c18b5de to 8f434cb Compare August 3, 2018 11:03
@clnperez
Copy link
Contributor

clnperez commented Aug 6, 2018

@albers is there a check done for image names anywhere? the same rules apply (though I admit I don't know much about how the bash-completion verification works).

@albers
Copy link
Collaborator Author

albers commented Aug 7, 2018

@clnperez So I can use an image name as a MANIFEST?
I successfully tried docker manifest inspect alpine:3.6, but I got an error with

$ docker manifest create alpine-list alpine:3.6 alpine:3.7 alpine:3.8
docker.io/library/alpine:3.6 is a manifest list

Can you please give me a working example fo a manifest create command?

@clnperez
Copy link
Contributor

clnperez commented Aug 7, 2018

There are some examples in the doc: https://docs.docker.com/engine/reference/commandline/manifest

Your issue is that alpine:3.6, alpine:3.7 and alpine:3.8 are all already manifest lists, and you can't make a recursive manifest list.

@albers
Copy link
Collaborator Author

albers commented Aug 7, 2018

@clnperez I found the examples not very helpful because they are not working code. There seem to be some prerequistes required for the commands to work apart from pulling down images and pushing them to a local registry.
I tried:

$ docker pull busybox:1.29.2-uclibc
$ docker tag busybox:1.29.2-uclibc localhost:5000/albers/busybox:1.29.2-uclibc
$ docker push localhost:5000/albers/busybox:1.29.2-uclibc
$ docker manifest create localhost:5000/albers/busybox:1.29.2 localhost:5000/albers/busybox:1.29.2-uclibc
no such manifest: localhost:5000/albers/busybox:1.29.2-uclibc

It's very hard to figure this out without understanding the relationship between images and manifests.
I assumed that each image has a manifest. Now it looks as some images are manifests. And it looks like manifests now can exist without images. What is a manifest list?

@albers albers force-pushed the completion-cli-experimental branch from 8f434cb to 6562929 Compare August 7, 2018 18:20
@clnperez
Copy link
Contributor

clnperez commented Aug 7, 2018

three things:

@albers albers force-pushed the completion-cli-experimental branch from 6562929 to 4e4f8b2 Compare August 7, 2018 20:20
@albers
Copy link
Collaborator Author

albers commented Aug 8, 2018

@clnperez Thanks very much for pointing me to your blog. This was exactly what I needed to understand the feature. I was able to create, annotate and push manifest lists.

@albers
Copy link
Collaborator Author

albers commented Aug 8, 2018

I added completion for manifest lists.
The completion is not perfect because there is no way to

  • list unpushed manifest lists.
  • get a list of all images with the manifest lists removed

As an approximation, I complete all images for both manifest lists and manifests.

@albers albers force-pushed the completion-cli-experimental branch from eb774e7 to 85baa0d Compare August 28, 2018 15:13
@albers
Copy link
Collaborator Author

albers commented Aug 28, 2018

Added support for completion of --os and --arch (legal values), rebased and sqashed some commits.

This is now ready for review.
ping @clnperez @thaJeztah

@albers albers changed the title [WIP] Add bash completion for experimental CLI commands (manifest) Add bash completion for experimental CLI commands (manifest) Aug 28, 2018
@clnperez
Copy link
Contributor

clnperez commented Aug 29, 2018

@albers This sounds reasonable to me. I might ask that we put a comment into the file you pulled those os & arch values from and in your PR that mentions that we should keep them in sync.

This preapares bash completion for more context sensitivity:

- experimental cli features
- orchestrator specific features

Also renames _daemon_ to _server_ where used in context of `docker version`
because the fields there are grouped unter _Server_.

Signed-off-by: Harald Albers <[email protected]>
This is needed for implementing bash completion for the `docker manifest`
command family.

Signed-off-by: Harald Albers <[email protected]>
@albers albers force-pushed the completion-cli-experimental branch from 85baa0d to 0fb4256 Compare August 30, 2018 06:55
@albers
Copy link
Collaborator Author

albers commented Aug 30, 2018

... put a comment into the file you pulled those os & arch values from and in your PR that mentions that we should keep them in sync.

@clnperez Done.

@clnperez
Copy link
Contributor

clnperez commented Sep 9, 2018

@vdemeester @thaJeztah Thoughts on this one?

Copy link
Collaborator

@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.

Still LGTM

@silvin-lubecki
Copy link
Contributor

silvin-lubecki commented Sep 10, 2018

Do we wait for #1355 to be merged ?

@albers
Copy link
Collaborator Author

albers commented Nov 20, 2018

@thaJeztah please review.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

sorry, thought I already LGTM'd this one 😅

@thaJeztah thaJeztah merged commit 1f6a1a4 into docker:master Nov 29, 2018
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Nov 29, 2018
@albers albers deleted the completion-cli-experimental branch November 29, 2018 17:16
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.

8 participants