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

Cobra completion v2 with CLI plugin support #3429

Merged
merged 1 commit into from May 12, 2022

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Feb 17, 2022

- 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 need dockerCli to list available values, and passing this will change method signature. Or we need to set a global util.* variable like kubernetes does

Root Command gets a custom ValidArgsFunction set so that we can list available CLI plugins and include them in the completions. Note: they appear after canonical commands, unsorted:

$ docker c [tab] [tab]
checkpoint  commit      compose     config      container   context     cp          create 
...
 ~ docker compose - [tab] [tab]
--ansi               (Control when to print ANSI control characters ("never"|"always"|"auto"))
--compatibility      (Run compose in backward compatibility mode)
--env-file           (Specify an alternate environment file.)
...

Note: support by plugin require docker/cli dependency to be updated and Metadata to explicitly declare Completion support (SchemaVersion 0.2.0)

- How to verify it
generate completion script by docker completion bash > /usr/local/etc/bash_completion.d/docker.sh and check completion within a new console

or directly invoke completion by:

$ docker __complete --context "de"
desktop-linux
default
:0
Completion ended with directive: ShellCompDirectiveDefault

demo: https://asciinema.org/a/cThdD50ILb3G5Ugx9q25XlTGo

- Description for the changelog
Introduce generation of shell completion command docker completion with dynamic completion

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

@ndeloof ndeloof force-pushed the cobra_completion_v2 branch 2 times, most recently from 4b68b6b to df0647c Compare February 17, 2022 10:23
@ndeloof ndeloof changed the title skeleton to adopt Cobra completion v2 Cobra completion v2 Feb 17, 2022
@codecov-commenter
Copy link

codecov-commenter commented Feb 17, 2022

Codecov Report

Merging #3429 (19ef618) into master (bfa9b40) will decrease coverage by 0.40%.
The diff coverage is 11.63%.

❗ Current head 19ef618 differs from pull request most recent head 003dc15. Consider uploading reports for the commit 003dc15 to get more accurate results

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

@@ -25,4 +25,6 @@ type Metadata struct {
// Experimental specifies whether the plugin is experimental.
// 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"`
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

return nil, cobra.ShellCompDirectiveError
}

return filterForCompletion(names, toComplete), cobra.ShellCompDirectiveDefault

Choose a reason for hiding this comment

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

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

)
}

func filterForCompletion(values []string, toComplete string) []string {

Choose a reason for hiding this comment

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

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

@ndeloof ndeloof force-pushed the cobra_completion_v2 branch 9 times, most recently from e7193c3 to 5a64693 Compare February 25, 2022 14:50
@ndeloof ndeloof changed the title Cobra completion v2 Cobra completion v2 with CLI plugin support Feb 25, 2022
ndeloof added a commit to ndeloof/compose that referenced this pull request Feb 25, 2022
(see docker/cli#3429)

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof ndeloof force-pushed the cobra_completion_v2 branch 3 times, most recently from 94d87b3 to 5a65670 Compare February 28, 2022 09:02
@ndeloof ndeloof force-pushed the cobra_completion_v2 branch 3 times, most recently from 09eeb1f to ae2b5da Compare March 15, 2022 16:46
@ndeloof ndeloof requested a review from thaJeztah May 10, 2022 13:40
@rumpl
Copy link
Member

rumpl commented May 11, 2022

Looks like all the comments were fixed, maybe we should merge this one?

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@thaJeztah thaJeztah added this to the 22.06.0 milestone May 12, 2022
@thaJeztah
Copy link
Member

LOL completion is funny; wondering if there's an option to prevent this (each <TAB> adds a new paused-container to the list; it probably completes automatically because there's only one match;

docker unpause paused-container paused-container paused-container paused-container paused-container paused-container paused-container paused-container

@thaJeztah
Copy link
Member

thaJeztah commented May 12, 2022

For the list of "improvements"; we should exclude default (built-in) networks;

docker network rm <TAB>
bridge                  docker_gwbridge         docker_mynetwork  host                    none

Also need to prevent docker network create from completing to local files

docker network create
# prints file list

And some tweaking for attach (probably shouldn't have the paused-container

docker container attach
cranky_varahamihira  paused-container     running-container

docker build now completes to directories only (nice!)

docker build
.circleci/         .idea/             cli-plugins/       demo/              e2e/               man/               scripts/           vendor/
.git/              build/             cmd/               dockerfiles/       experimental/      opts/              service/
.github/           cli/               contrib/           docs/              internal/          

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, thanks!

@thaJeztah thaJeztah merged commit a09e61a into docker:master May 12, 2022
ndeloof added a commit to ndeloof/compose that referenced this pull request May 13, 2022
(see docker/cli#3429)

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof added a commit to ndeloof/compose that referenced this pull request May 13, 2022
(see docker/cli#3429)

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof added a commit to ndeloof/compose that referenced this pull request May 13, 2022
(see docker/cli#3429)

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof added a commit to ndeloof/compose that referenced this pull request May 13, 2022
(see docker/cli#3429)

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
ndeloof added a commit to ndeloof/compose that referenced this pull request May 13, 2022
(see docker/cli#3429)

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
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.

None yet

5 participants