Add top-level --completion flag, and integrate completion for plugins #3254

thaJeztah · 2021-08-15T19:19:34Z

This patch adds a new top-level --completion flag, which allows the user to
print the completion script for their shell of choice.

We currently maintain hand-written completion scripts for bash, fish, and zsh,
of which the bash script is in a good state, but the fish and zsh scripts are
less well maintained.

We should review the quality of the fish and zsh scripts, and decide wether or
not we want to use the hand-written versions for those, or to replace them with
the auto-generated completion provided by Cobra.

This patch embeds the hand-written completion scripts using Go 1.16's "embed"
functionality, but also adds options to try/test/compare the Cobra completion
scripts. Currently, this patch allows for the following:

docker --completion=bash        outputs the hand-written bash completion script
docker --completion=bash-cobra  outputs cobra's generated completion script
docker --completion=fish        outputs the hand-written fish completion script
docker --completion=fish-cobra  outputs cobra's generated completion script
docker --completion=powershell  outputs cobra's generated completion script
docker --completion=zsh         outputs the hand-written bash completion script
docker --completion=zsh-cobra   outputs cobra's generated completion script

While the hand-written bash completion script has definitions for commands and
options provided by the main CLI, it does not provide completions for commands
and options provided by plug-ins. Commits 1148163
and 5a8d7d5 (#3158) added support for completions for
the compose cli plug-in, but do so by directly calling the plug-in binary.

Cobra's completion scripts use two hidden subcommands that provide dynamic
generating of completion scripts: __complete and __completeNoDesc. This
patch adds support for both these subcommands, and implements code to extend
these commands to be plug-in aware: if either of those commands request completion
for an unknown command, the cli will look if a plug-in is install that provides
the command, and will call the __complete or __completeNoDesc command on the
plug-in.

With that, we are able to automatically provide completion scripts for all plug-
ins. For example:

docker __complete compose r
restart	Restart containers
rm	Removes stopped service containers
run	Run a one-off command on a service.
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

docker __complete scan --
--accept-license	Accept using a third party scanning provider
--dependency-tree	Show dependency tree with scan results
--exclude-base	Exclude base image from vulnerability scanning (requires --file)
--file	Dockerfile associated with image, provides more detailed results
--file=	Dockerfile associated with image, provides more detailed results
--group-issues	Aggregate duplicated vulnerabilities and group them to a single one (requires --json)
--json	Output results in JSON format
--login	Authenticate to the scan provider using an optional token (with --token), or web base token if empty
--reject-license	Reject using a third party scanning provider
--severity	Only report vulnerabilities of provided level or higher (low|medium|high)
--severity=	Only report vulnerabilities of provided level or higher (low|medium|high)
--token	Authentication token to login to the third party scanning provider
--token=	Authentication token to login to the third party scanning provider
--version	Display version of the scan plugin
:0
Completion ended with directive: ShellCompDirectiveDefault

docker __completeNoDesc scan --
--accept-license
--dependency-tree
--exclude-base
--file
--file=
--group-issues
--json
--login
--reject-license
--severity
--severity=
--token
--token=
--version
:0
Completion ended with directive: ShellCompDirectiveDefault

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2021-08-15T19:20:47Z

contrib/completion/bash/docker

-_docker_compose() {
-	local completionCommand="__completeNoDesc"
-	local resultArray=($COMPOSE_PLUGIN_PATH $completionCommand compose)
+__completeNoDesc() {


I need to check if I updated this correctly. The intent is to:

change /usr/local/lib/docker/cli-plugins/docker-compose __completeNoDesc <command> to docker __completeNoDesc <command>

the same should now work for all plugins, so, we should be able to just use all plugins:

docker info --format '{{range .ClientInfo.Plugins}}{{ .Name }} {{end}}' buildx compose scan

thaJeztah · 2021-08-15T20:08:31Z

@silvin-lubecki @albers @ulyssessouza ptal

codecov-commenter · 2021-08-15T20:12:42Z

Codecov Report

Merging #3254 (1c332c7) into master (304a2dc) will decrease coverage by 0.09%.
The diff coverage is 6.06%.

@@            Coverage Diff             @@
##           master    #3254      +/-   ##
==========================================
- Coverage   57.99%   57.90%   -0.10%     
==========================================
  Files         302      302              
  Lines       21748    21774      +26     
==========================================
- Hits        12613    12608       -5     
- Misses       8212     8242      +30     
- Partials      923      924       +1

silvin-lubecki · 2021-08-16T09:33:22Z

contrib/completion/bash/docker

 	COMPREPLY=( $(compgen -W "${result}" -- "$current") )
 }

+_docker_compose() {


typo: ~~_docker_compose~~ _docker_buildx

doh!

Still need to verify if this works as expected as well (didn't try yet TBH, as I ran out of steam 😅)

silvin-lubecki · 2021-08-16T09:34:13Z

contrib/completion/bash/docker

-	if [ -f "$COMPOSE_PLUGIN_PATH" ] ; then
-		known_plugin_commands+=("compose")
-	fi
+	PLUGINS_INSTALLED=$(docker info --format '{{range .ClientInfo.Plugins}}{{ .Name }} {{end}}')


question: shouldn't we have a curated list here instead?

I don't think so. Every plugin should have the posibility to hook into completion.

Perhaps; otoh, if the plugin is there, adding completion for it doesn't really harm I guess.
We should consider having a v0.0.2 version of the metadata that indicates that the plugin supports the __complete subcommands (although current code should handle it gracefully).

Actually; currently that won't work, because we have the dedicated _docker_<plugin name> functions 🤦

silvin-lubecki · 2021-08-16T09:36:14Z

cmd/docker/docker.go

 	}
-	opts, flags, helpCmd = cli.SetupRootCommand(cmd)
+	opts, flags, helpCmd := cli.SetupRootCommand(cmd)
+	flags.String("completion", "", "Print the shell completion script for the specified shell (bash, fish, powershell, or zsh) and quit")


question: we mention powershell here but it's not in the completion map in completion.go. I guess it would fail then ? 🤔

Ok my bad, powershell is fully generated by cobra 👍

No, it falls back to the auto-generated Cobra completion

albers · 2021-08-16T09:48:42Z

contrib/completion/bash/docker

 	COMPREPLY=( $(compgen -W "${result}" -- "$current") )
 }

+_docker_compose() {


This should be generic.
Assuming that a plugin contributes only one top level command, and that this command equals its name, we can check whether the command is part of $PLUGINS_INSTALLED right before calling

cli/contrib/completion/bash/docker

Line 5663 in 304a2dc

local completions_func=_docker_${command//-/_}

and if so, call a generic plugin completion instead.

Ah, thanks! Yes, I was assuming something like that was possible. That said, plugins do provide subcommands, in which case it should call (e.g. for docker buildx imagetools)

(looks like a "" is needed to force it to look up subcommands);

docker __complete buildx imagetools "" create Create a new image based on source images inspect Show details of image in the registry :4 Completion ended with directive: ShellCompDirectiveNoFileComp

Or, for flags (--);

docker __complete buildx imagetools -- --builder Override the configured builder instance :4 Completion ended with directive: ShellCompDirectiveNoFileComp

I'm a big fan of a generic plugin-completion solution. That being said, I think care is needed here.

The $PLUGINS_INSTALLED list returns all cli plugins, not just the ones provided by docker. I don't think it would be right to call docker __completeNoDesc <anyPlugin> as those plugins may not support the __completeNoDesc command and may instead simply ignore it and execute whatever functionality they normally perform.

This was already hinted at by @thaJeztah earlier with the idea of "a v0.0.2 version of the metadata that indicates that the plugin supports the __complete subcommands" (#3254 (comment)), so sorry if I'm repeating things; I thought it was worth mentioning as it has been an important point in the discussion about how to support plugin completion for kubectl.

albers · 2021-08-16T09:59:05Z

I'd vote for a dedicated command for this functionality.

In my understanding, an option should only modify how a given command works, but not switch to conceptually different operation modes (disguised commands).

There are some well-known exceptions like --help and --version.
I don't feel --completion falls into this category. This is emphasized by the fact that the option needs a parameter in order to work.

Examples for similar commands that have dedicated completion commands:

vagrant autocomplete
kubectl completion bash
minikube completion bash

thaJeztah · 2021-08-16T10:06:06Z

I'd vote for a dedicated command for this functionality.

I was a bit on the fence on this one, being that this (in most cases) will be a "set and forget" option (users would either use it once write to a file, or set it in their .profile or .bashrc). Making it a command would make it show up more prominently in the --help output (haven't tried, but probably it shows up under Management commands, but we can of course change that).

The kubectl completion uses the default Cobra functionality, which indeed is named completion (that said, we don't have to follow their naming, so we could name it anything)

thaJeztah · 2021-08-16T10:06:24Z

Hmm... one thing to look into; it looks like the code doesn't match up;

When calling the plugin directly:

/usr/local/lib/docker/cli-plugins/docker-buildx __complete build
build	Start a build
:0
Completion ended with directive: ShellCompDirectiveDefault

When calling it through the cli:

./build/docker __complete buildx build
build	Start a build
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

Where :4 (ShellCompDirectiveNoFileComp) is documented as:

// ShellCompDirectiveNoFileComp indicates that the shell should not provide
// file completion even when no completion is provided.
// This currently does not work for zsh or bash < 4
ShellCompDirectiveNoFileComp

And :0 (ShellCompDirectiveDefault) is documented as:

// ShellCompDirectiveDefault indicates to let the shell perform its default
// behavior after completions have been provided.
// This one must be last to avoid messing up the iota count.
ShellCompDirectiveDefault ShellCompDirective = 0

From the above, I think it should return :0, otherwise it would not do further completions?

This patch adds a new top-level `--completion` flag, which allows the user to print the completion script for their shell of choice. We currently maintain hand-written completion scripts for bash, fish, and zsh, of which the bash script is in a good state, but the fish and zsh scripts are less well maintained. We should review the quality of the fish and zsh scripts, and decide wether or not we want to use the hand-written versions for those, or to replace them with the auto-generated completion provided by Cobra. This patch embeds the hand-written completion scripts using Go 1.16's "embed" functionality, but also adds options to try/test/compare the Cobra completion scripts. Currently, this patch allows for the following: docker --completion=bash outputs the hand-written bash completion script docker --completion=bash-cobra outputs cobra's generated completion script docker --completion=fish outputs the hand-written fish completion script docker --completion=fish-cobra outputs cobra's generated completion script docker --completion=powershell outputs cobra's generated completion script docker --completion=zsh outputs the hand-written bash completion script docker --completion=zsh-cobra outputs cobra's generated completion script While the hand-written bash completion script has definitions for commands and options provided by the main CLI, it does not provide completions for commands and options provided by plug-ins. Commits 1148163 and 5a8d7d5 added support for completions for the compose cli plug-in, but do so by directly calling the plug-in binary. Cobra's completion scripts use two hidden subcommands that provide dynamic generating of completion scripts: `__complete` and `__completeNoDesc`. This patch adds support for both these subcommands, and implements code to extend these commands to be plug-in aware: if either of those commands request completion for an unknown command, the cli will look if a plug-in is install that provides the command, and will call the `__complete` or `__completeNoDesc` command on the plug-in. With that, we are able to automatically provide completion scripts for all plug- ins. For example: docker __complete compose r restart Restart containers rm Removes stopped service containers run Run a one-off command on a service. :4 Completion ended with directive: ShellCompDirectiveNoFileComp docker __complete scan -- --accept-license Accept using a third party scanning provider --dependency-tree Show dependency tree with scan results --exclude-base Exclude base image from vulnerability scanning (requires --file) --file Dockerfile associated with image, provides more detailed results --file= Dockerfile associated with image, provides more detailed results --group-issues Aggregate duplicated vulnerabilities and group them to a single one (requires --json) --json Output results in JSON format --login Authenticate to the scan provider using an optional token (with --token), or web base token if empty --reject-license Reject using a third party scanning provider --severity Only report vulnerabilities of provided level or higher (low|medium|high) --severity= Only report vulnerabilities of provided level or higher (low|medium|high) --token Authentication token to login to the third party scanning provider --token= Authentication token to login to the third party scanning provider --version Display version of the scan plugin :0 Completion ended with directive: ShellCompDirectiveDefault docker __completeNoDesc scan -- --accept-license --dependency-tree --exclude-base --file --file= --group-issues --json --login --reject-license --severity --severity= --token --token= --version :0 Completion ended with directive: ShellCompDirectiveDefault Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

marckhouzam · 2022-02-22T11:14:26Z

cmd/docker/docker.go

+			// Handle shell completion for plugin commands by forwarding the
+			// arguments to the plugin. Supporting completion is currently
+			// optional for plugins, so we ignore errors (if any).
+			if len(args) > 0 && cmd.Name() == cobra.ShellCompRequestCmd || cmd.Name() == cobra.ShellCompNoDescRequestCmd {


This is fine, but FYI, the "__completeNoDesc" form is an alias of the same Cobra command (__complete). This means that in both cases cmd.Name() will be cobra.ShellCompRequestCmd, so you could simply check for that.

marckhouzam · 2022-02-22T13:28:02Z

contrib/completion/bash/docker

-	local completionCommand="__completeNoDesc"
-	local resultArray=($COMPOSE_PLUGIN_PATH $completionCommand compose)
+__completeNoDesc() {
+	local resultArray=(docker __completeNoDesc $1)


Instead of hard-coding docker you may want to use ${words[0]} here.
This will call the docker binary that the user is actually using. For example:

./build/docker buildx s[tab][tab]

would call ./build/docker __completeNoDesc buildx s instead of the docker __completeNoDesc buildx s, which I believe is what the user would want.

marckhouzam · 2022-02-22T13:36:48Z

contrib/completion/bash/docker

 	done
 	local result=$(eval "${resultArray[*]}" 2> /dev/null | grep -v '^:[0-9]*$')

 	COMPREPLY=( $(compgen -W "${result}" -- "$current") )


Using grep above is risky since this is a sourced script and therefore will take whatever grep alias may be in the user's environment. My grep alias prefixes the output with the line number (grep -n) which messes up completion.

I've had the same issue in my completion scripts 😄
You can use \grep, but it would be even better to use bash constructs to be more portable.
In Cobra, we use

# Remove the directive result=${result%:*}

marckhouzam · 2022-02-22T14:07:12Z

contrib/completion/bash/docker

 	COMPREPLY=( $(compgen -W "${result}" -- "$current") )
 }

+_docker_compose() {


I'm a big fan of a generic plugin-completion solution. That being said, I think care is needed here.

The $PLUGINS_INSTALLED list returns all cli plugins, not just the ones provided by docker. I don't think it would be right to call docker __completeNoDesc <anyPlugin> as those plugins may not support the __completeNoDesc command and may instead simply ignore it and execute whatever functionality they normally perform.

This was already hinted at by @thaJeztah earlier with the idea of "a v0.0.2 version of the metadata that indicates that the plugin supports the __complete subcommands" (#3254 (comment)), so sorry if I'm repeating things; I thought it was worth mentioning as it has been an important point in the discussion about how to support plugin completion for kubectl.

albers · 2022-02-27T20:26:19Z

I don't think it would be right to call docker __completeNoDesc as those plugins may not support the __completeNoDesc command and may instead simply ignore it and execute whatever functionality they normally perform.

I think it should be sufficient to check the exit code of the __complete call and if it is not 0 then discard the plugin's output.

plugin with completion support

$ /usr/libexec/docker/cli-plugins/docker-scan __complete --c
--config	Location of client config files
--context	Name of the context to use to connect to the daemon (overrides DOCKER_HOST env var and default context set with "docker context use")
:4
Completion ended with directive: ShellCompDirectiveNoFileComp
$ echo $?
0

plugin without completion support

$ /usr/libexec/docker/cli-plugins/docker-app __complete --c
unknown flag: --c
See 'docker --help'.
[...]
$ echo $?
125

marckhouzam · 2022-02-28T00:45:33Z

I think it should be sufficient to check the exit code of the __complete call and if it is not 0 then discard the plugin's output.

We started with a similar point in the kubectl discussion but after iterating we realized it was problematic. Say a docker-cli plugin is a simple bash script. One could imagine a docker-prune plugin like:

#!/bin/bash

# handle docker plugin metadata first

docker system prune

Since the plugin ignores arguments, calling it with the __complete command would still cause the plugin to execute and do the pruning. It's for such cases that knowing in advance if completion is supported becomes important.

thaJeztah added impact/changelog kind/feature status/2-code-review area/plugins impact/documentation area/completion labels Aug 15, 2021

thaJeztah commented Aug 15, 2021

View reviewed changes

thaJeztah force-pushed the add_completion branch 3 times, most recently from 1ab865b to 4ae8ae1 Compare August 15, 2021 20:02

thaJeztah force-pushed the add_completion branch 2 times, most recently from 990d54e to f41bfc8 Compare August 16, 2021 09:29

silvin-lubecki reviewed Aug 16, 2021

View reviewed changes

albers reviewed Aug 16, 2021

View reviewed changes

thaJeztah force-pushed the add_completion branch from f41bfc8 to 1c332c7 Compare August 16, 2021 10:08

mcornella mentioned this pull request Sep 15, 2021

Docker compose is now part of docker ohmyzsh/ohmyzsh#9831

Closed

thaJeztah mentioned this pull request Feb 17, 2022

Cobra completion v2 with CLI plugin support #3429

Merged

marckhouzam reviewed Feb 22, 2022

View reviewed changes

Add top-level --completion flag, and integrate completion for plugins #3254

Are you sure you want to change the base?

Add top-level --completion flag, and integrate completion for plugins #3254

Conversation

thaJeztah commented Aug 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 15, 2021

Uh oh!

codecov-commenter commented Aug 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albers commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Aug 16, 2021

Uh oh!

thaJeztah commented Aug 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albers commented Feb 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marckhouzam commented Feb 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thaJeztah commented Aug 15, 2021 •

edited

Loading

codecov-commenter commented Aug 15, 2021 •

edited

Loading

albers commented Aug 16, 2021 •

edited

Loading

albers commented Feb 27, 2022 •

edited

Loading