Cobra completion v2 with CLI plugin support#3429
Conversation
4b68b6b to
df0647c
Compare
df0647c to
3be87bb
Compare
Codecov Report
@@ Coverage Diff @@
## master #3429 +/- ##
==========================================
- Coverage 59.47% 59.06% -0.41%
==========================================
Files 287 288 +1
Lines 24174 24372 +198
==========================================
+ Hits 14377 14395 +18
- Misses 8931 9105 +174
- Partials 866 872 +6 |
cli-plugins/manager/metadata.go
Outdated
| // Deprecated: experimental features are now always enabled in the CLI | ||
| Experimental bool `json:",omitempty"` | ||
| // Completions specifies whether the plugin supports command completion | ||
| Completions bool `json:",omitempty"` |
There was a problem hiding this comment.
Perhaps we should consider updating the SchemaVersion as well
// SchemaVersion describes the version of this struct. Mandatory, must be "0.1.0"
Do you think we can combine this PR with #3254 ? (feel free to take that branch of course, if there's useful bits in it)
There was a problem hiding this comment.
I'm not sure about #3254. I understand we want to keep backward compatibility for users, but on the other hand using cobra completion v2 bring more flexibility and CLI plugins support, so should be preferred. If we support both, we will have to offer double maintenance...
cmd/docker/completions.go
Outdated
| return nil, cobra.ShellCompDirectiveError | ||
| } | ||
|
|
||
| return filterForCompletion(names, toComplete), cobra.ShellCompDirectiveDefault |
There was a problem hiding this comment.
If there are no context that match what the user has typed, do you want the shell to suggest file name?
If not, you should return cobra.ShellCompDirectiveNoFileComp. For example:
$ docker --context D[tab][tab] # There is a context starting with 'D'
Development
$ docker --context Do[tab][tab] # There is NO context starting with 'Do' so we would get file completion
Documents/ Dockerfile Dockerfile.new Dockerfile.ori
cmd/docker/completions.go
Outdated
| ) | ||
| } | ||
|
|
||
| func filterForCompletion(values []string, toComplete string) []string { |
There was a problem hiding this comment.
You may want to consider not filtering on toComplete but always returning all choices. Most implementations do the filtering, but with the latest version of Cobra, it is safe not to do it. The advantage is that it allows for fuzzy matching for some shells. In this case, fuzzy matching means that the shell can match what the user has typed not just as a prefix.
So let's say you have contexts test-dev, test-prod, test-local, and the user types prod, zsh and fish shells would be smart and match on test-prod. This improves the user experience. As for bash and powershell that don't support fuzzy matching, they will filter on prefix automatically.
If you want more details, you can have a look at how we did this for helm: helm/helm#10513
e7193c3 to
5a64693
Compare
(see docker/cli#3429) Signed-off-by: Nicolas De Loof <[email protected]>
94d87b3 to
5a65670
Compare
09eeb1f to
ae2b5da
Compare
|
I think this is a good start Given that this provides an additional way to provide completion scripts (it's definitely not (yet) on par with the existing bash completion - although the other shells were less well-maintained, so perhaps for those it's "on-par"), I'm ok with including this, and then work on improving. I did find some issues (details below), perhaps some of them are easy fixes (let me know if they are). Here's what I tried (running within the dev container ( apk add --no-cache -qq bash-completion
. /etc/profile.d/bash_completion.sh
source <(docker completion bash)Created some containers in some states; make binary
docker create --name created-container busybox
docker run --name stopped-container busybox
docker run -dit --name running-container busybox
docker run -dit --name paused-container busybox
docker pause paused-containerRunning docker rm <TAB> <TAB>
created-container paused-container running-container stopped-containerAnd docker stop <TAB> <TAB>
paused-container running-containerBut docker start <TAB> <TAB>
created-container paused-container running-container stopped-containerSimilar for docker pause <TAB> <TAB>
paused-container running-container
docker unpause <TAB> <TAB>
paused-container running-containerTab completion for docker run --
Display all 100 possibilities? (y or n)
--add-host --cpu-rt-period --device-write-iops --health-interval --kernel-memory --name --quiet --tty
--attach --cpu-rt-runtime --disable-content-trust --health-retries --label --network --read-only --ulimit
--blkio-weight --cpu-shares --dns --health-start-period --label-file --network-alias --restart --user
--blkio-weight-device --cpus --dns-option --health-timeout --link --no-healthcheck --rm --userns
--cap-add --cpuset-cpus --dns-search --hostname --link-local-ip --oom-kill-disable --runtime --uts
--cap-drop --cpuset-mems --domainname --init --log-driver --oom-score-adj --security-opt --volume
--cgroup-parent --detach --entrypoint --interactive --log-opt --pid --shm-size --volume-driver
--cgroupns --detach-keys --env --io-maxbandwidth --mac-address --pids-limit --sig-proxy --volumes-from
--cidfile --device --env-file --io-maxiops --memory --platform --stop-signal --workdir
--cpu-count --device-cgroup-rule --expose --ip --memory-reservation --privileged --stop-timeout
--cpu-percent --device-read-bps --gpus --ip6 --memory-swap --publish --storage-opt
--cpu-period --device-read-iops --group-add --ipc --memory-swappiness --publish-all --sysctl
--cpu-quota --device-write-bps --health-cmd --isolation --mount --pull --tmpfsCompletion of flag values may need some improvement, for example, when trying if docker run --network=<TAB>
.DS_Store .golangci.yml LICENSE VERSION demo/ gometalinter.json.tmp service/
.circleci/ .idea/ MAINTAINERS build/ docker-bake.hcl internal/ suggestions.patch
...For a boolean flag (I picked docker run --rm <TAB>
<none>:<none> docker:19.03-dind golang:latest quay.io/centos/centos:stream8
...However, it also completed to For commands that don't take an argument, such as docker volume ls <TAB>
.DS_Store .golangci.yml LICENSE VERSION demo/ gometalinter.json.tmp service/
...I guess we can take all commands that have And the reverse docker build
.DS_Store .golangci.yml LICENSE VERSION demo/ gometalinter.json.tmp service/
...
.gitignore TESTING.md contrib/ experimental/ scripts/ vendor.sum |
cli/command/config/cmd.go
Outdated
| } | ||
|
|
||
| // CompletionFn offers completion for swarm secrets | ||
| func CompletionFn(dockerCli command.Cli) command.CompletionFn { |
There was a problem hiding this comment.
One thing; so it looks like all completion functions (with the exception of image.CompletionFn() and container.CompletionFn()) are only used within the package itself;
I generally like to keep things un-exported, unless needed; perhaps we should
- un-export all of them (except for the
imageandcontainerones) - should we have more descriptive names (for the exported ones at least)? (e.g.,
CompleteNames()?); mostly wondering if there will be more "specialized" completion functions (per my earlier comments w.r.t. "in specific states" etc), in which case a more descriptive name would help.
574c1cb to
b273ef1
Compare
|
@thaJeztah I've addressed some of the reported completion issue, there's many places we can improve. cmd.RegisterFlagCompletionFunc(
"network",
network.CompleteNames(dockerCli),
)but there's many other flags to address :) |
|
I was about to give this one another look, but I see there's an issue in the latest iteration, @ndeloof |
|
moved completion functions to |
cli/command/completion/functions.go
Outdated
| // CompleteContainerNames offers completion for container names and IDs | ||
| // By default, only names are returned. | ||
| // Set DOCKER_COMPLETION_SHOW_CONTAINER_IDS=yes to also complete IDs. | ||
| func CompleteContainerNames(dockerCli command.Cli, all bool, filters ...func(types.Container) bool) ValidArgsFn { |
There was a problem hiding this comment.
I'm ok with keeping this for now, and iterate on this, but somewhat on the fence on the bool argument (which could also be a "filter" / "option") and doing the filtering client-side
I think most (if not all) of the filtering we need can be done with a --filter (although in some cases, we'd have to pass multiple filters). For example;
docker ps --filter status=exited --filter status=created
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
7b1647808d7b busybox "sh" 56 minutes ago Exited (0) 56 minutes ago stopped-container
2ee1ae95ecf4 busybox "sh" 56 minutes ago Created created-containerWe could still look at implementing != as filter (--filter status!=running), but in most cases, we'd still have to be more specific (e.g. a paused container cannot be started, so only excluding running likely won't work)
|
Looks like all the comments were fixed, maybe we should merge this one? |
Signed-off-by: Nicolas De Loof <[email protected]>
|
LOL completion is funny; wondering if there's an option to prevent this (each docker unpause paused-container paused-container paused-container paused-container paused-container paused-container paused-container paused-container |
|
For the list of "improvements"; we should exclude default (built-in) networks; docker network rm <TAB>
bridge docker_gwbridge docker_mynetwork host noneAlso need to prevent docker network create
# prints file listAnd some tweaking for docker container attach
cranky_varahamihira paused-container running-container
docker build
.circleci/ .idea/ cli-plugins/ demo/ e2e/ man/ scripts/ vendor/
.git/ build/ cmd/ dockerfiles/ experimental/ opts/ service/
.github/ cli/ contrib/ docs/ internal/ |
- What I did
Updated Cobra to 1.3.0 and started adopting Completion v2 mechanism with dynamic flag completion
Longer terms plan is to be able to fully generate completion for all commands, and replace the hand-written completion script
- How I did it
Enabled completion command and introduced
registerCompletionFunc..I would prefer this is all set by
SetupRootCommand, but we needdockerClito list available values, and passing this will change method signature. Or we need to set a globalutil.*variable like kubernetes doesRoot Command gets a custom
ValidArgsFunctionset so that we can list available CLI plugins and include them in the completions. Note: they appear after canonical commands, unsorted:Note: support by plugin require docker/cli dependency to be updated and Metadata to explicitly declare
Completionsupport (SchemaVersion 0.2.0)- How to verify it
generate completion script by
docker completion bash > /usr/local/etc/bash_completion.d/docker.shand check completion within a new consoleor directly invoke completion by:
demo: https://asciinema.org/a/cThdD50ILb3G5Ugx9q25XlTGo
- Description for the changelog
Introduce generation of shell completion command
docker completionwith dynamic completion- A picture of a cute animal (not mandatory but encouraged)