-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Improve Cobra completions for run
and create
#5580
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5580 +/- ##
==========================================
+ Coverage 59.21% 59.50% +0.28%
==========================================
Files 342 346 +4
Lines 29083 29365 +282
==========================================
+ Hits 17222 17474 +252
- Misses 10889 10916 +27
- Partials 972 975 +3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is amazing work, thank you!! I left quite some comments, but welcome your feedback/thoughts.
I think the most notable one that could reduce a lot of boiler plating, is the comment on (perhaps could be a utility) setting a "default" NoComplete
on all flags; that way we wouldn't have to set the NoComplete
for all flags that don't have one. (Although maybe there's something to be said for explicitly setting "no completions here)
cli/command/container/completion.go
Outdated
@@ -57,6 +57,7 @@ var restartPolicies = []string{ | |||
|
|||
// addCompletions adds the completions that `run` and `create` have in common. | |||
func addCompletions(cmd *cobra.Command, dockerCli command.Cli) { | |||
_ = cmd.RegisterFlagCompletionFunc("add-host", completion.NoComplete) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, yes, this is something I was looking at as well. I was considering if we could set a default, then optionally override, but Cobra currently doesn't allow for a completion to be replaced once set;
cli/vendor/github.com/spf13/cobra/completions.go
Lines 143 to 146 in 32ff200
if _, exists := flagCompletionFunctions[flag]; exists { | |
return fmt.Errorf("RegisterFlagCompletionFunc: flag '%s' already registered", flagName) | |
} | |
flagCompletionFunctions[flag] = f |
In that case, if the flag has no completion, it returns here;
cli/vendor/github.com/spf13/cobra/completions.go
Lines 434 to 440 in 32ff200
if !finalCmd.DisableFlagParsing { | |
// If DisableFlagParsing==false, we have completed the flags as known by Cobra; | |
// we can return what we found. | |
// If DisableFlagParsing==true, Cobra may not be aware of all flags, so we | |
// let the logic continue to see if ValidArgsFunction needs to be called. | |
return finalCmd, completions, directive, nil | |
} |
Which returns with ShellCompDirectiveNoFileComp
directive = ShellCompDirectiveNoFileComp |
I think ideally Cobra would allow setting a custom default for flags, but perhaps a somewhat dirty, but functional one would be to do it ourselves, and set a default at the very end of functions that constructed a command, e.g. at;
cli/cli/command/container/create.go
Line 92 in 32ff200
} |
flags.VisitAll(func(flag *pflag.Flag) {
// Set a default completion function if none was set. We don't look
// up if it does already have one set, because Cobra does this for
// us, and returns an error (which we ignore for this reason).
_ = cmd.RegisterFlagCompletionFunc(flag.Name, completion.NoComplete)
})
return cmd
I was a bit on the fence on using VisitAll
, but it looks like there's many places where Cobra already does, so maybe not too bad, WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not find a way to set a default ShellCompDirective
for silencing the numerouns flags with inapproptiate file completions.
IMHO this should be handled in Cobra. I'll open a feature request for this.
Your workaround looks very promising, I'll give it a try.
@@ -59,6 +59,7 @@ var restartPolicies = []string{ | |||
func addCompletions(cmd *cobra.Command, dockerCli command.Cli) { | |||
_ = cmd.RegisterFlagCompletionFunc("add-host", completion.NoComplete) | |||
_ = cmd.RegisterFlagCompletionFunc("annotation", completion.NoComplete) | |||
_ = cmd.RegisterFlagCompletionFunc("attach", completion.FromList("stderr", "stdin", "stdout")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh; I was working on this one, when I discovered completion doesn't work for specifying --attach
multiple times.
I started looking "why???" because it does work for some other flags we define multiple times, which is when I discovered the reason; Cobra has some "magic" values for flag "types"; if the flag-type ends with Slice
or Array
it considers that it can be used multiple times. If not, it considers that the flag can only be set once and it removes it from the suggestions;
cli/vendor/github.com/spf13/cobra/completions.go
Lines 402 to 408 in 32ff200
if !flag.Changed || | |
strings.Contains(flag.Value.Type(), "Slice") || | |
strings.Contains(flag.Value.Type(), "Array") { | |
// If the flag is not already present, or if it can be specified multiple times (Array or Slice) | |
// we suggest it as a completion | |
completions = append(completions, getFlagNameCompletions(flag, toComplete)...) | |
} |
In our case, some types use list
as name, which isn't matched. I'm considering contributing a patch to Cobra to make it somewhat smarter;
- make the matching case-insensitive
- possibly add
list
to the magic strings
But also to have it match SliceValue
, which is an interface defined by the pflag
module used by Cobra for flags. I think it's an omission that it doesn't consider that interface;
cli/vendor/github.com/spf13/pflag/flag.go
Lines 196 to 203 in 32ff200
type SliceValue interface { | |
// Append adds the specified value to the end of the flag value list. | |
Append(string) error | |
// Replace will fully overwrite any data currently in the flag value list. | |
Replace([]string) error | |
// GetSlice returns the flag value list as an array of strings. | |
GetSlice() []string | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, now I understand.
I was also wondering why the events --filter
completion only works once.
cli/command/container/completion.go
Outdated
_ = cmd.RegisterFlagCompletionFunc("dns", completion.NoComplete) | ||
_ = cmd.RegisterFlagCompletionFunc("dns-option", completion.NoComplete) | ||
_ = cmd.RegisterFlagCompletionFunc("dns-search", completion.NoComplete) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly "note to self" - I think it's fine to disable completion for these for now.
- For
dns-option
we could at some point consider adding the list from https://man7.org/linux/man-pages/man5/resolv.conf.5.html - For
dns-search
I wonder how much sense it would make to complete based on the current host's settings. That would not make sense for a remote daemon, but wondering if it's still useful for situations where the daemon runs locally. - (same for
dns
)
@@ -138,6 +138,11 @@ func addCompletions(cmd *cobra.Command, dockerCli command.Cli) { | |||
_ = cmd.RegisterFlagCompletionFunc("workdir", completion.NoComplete) | |||
} | |||
|
|||
// completeDetachKeys implements shell completion for the `--detach-keys` option of `run` and `create`. | |||
func completeDetachKeys(_ *cobra.Command, _ []string, _ string) ([]string, cobra.ShellCompDirective) { | |||
return []string{"ctrl-"}, cobra.ShellCompDirectiveNoSpace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self; I need to lookup if there's other special ones we define (meta-
, alt-
, possibly others)
110bb3d
to
ef6cca9
Compare
run
and create
run
and create
@thaJeztah Thanks very much for your mentoring. It's very much appreciated. I think I'm through with your review comments, please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Let me know when I can squash the commits into a more relevant set of commits. |
Thank you so much for this @albers! I'm thinking we could squash |
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>
4378ab4
to
32957b3
Compare
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>
Signed-off-by: Harald Albers <github@albersweb.de>
Signed-off-by: Harald Albers <github@albersweb.de>
Credits to thaJeztah Signed-off-by: Harald Albers <github@albersweb.de>
32957b3
to
06260e6
Compare
@laurazard The commits are now squashed to a sequence of meaningful, self-contained commits. @thaJeztah would you like to take another look at this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-LGTM. Thanks!
There was a problem hiding this 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 did 🙈 🙈 🙈
- What I did
Added Cobra completion for all options of
docker run
anddocker create
.- How I did it
Provided
FlagCompletionFunc
s for all options except forThe reference for my work was the legacy bash completion.
Features that I did not recreate:
--user
- How to verify it
make completion
, then try completions of variousdocker run
anddocker create
commands.. ./contrib/completion/bash/docker
. then execute the same completions for reference.- Description for the changelog
Improved completions for
docker run
anddocker create
.