Improve Cobra completions for run and create

[albers]

- What I did
Added Cobra completion for all options of docker run and docker create.

- How I did it
Provided FlagCompletionFuncs for all options except for

• boolean flags (default completion works)
• flags with file completions (default completion works)
• flags that already had a completion function.

The reference for my work was the legacy bash completion.
Features that I did not recreate:

• completion for --user
• completion for the individual log driver options
• completion of non-built-in drivers

- How to verify it

1. In a dev container, issue make completion, then try completions of various docker run and docker create commands.
2. In another terminal, exec into the container, install the legacy completion with . ./contrib/completion/bash/docker. then execute the same completions for reference.

- Description for the changelog
Improved completions for docker run and docker create.

Ported some completions from the bash completion to the new cobra based completion.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 43.50282% with 100 lines in your changes missing coverage. Please review.

Project coverage is 59.50%. Comparing base (61baf2a) to head (06260e6).
Report is 82 commits behind head on master.

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     

[thaJeztah]

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)

[thaJeztah]

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

cli/vendor/github.com/spf13/cobra/completions.go

Line 427 in 32ff200

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?

[albers]

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.

[thaJeztah]

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
	}

[albers]

Ah, now I understand.
I was also wondering why the events --filter completion only works once.

[thaJeztah]

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)

[thaJeztah]

Note to self; I need to lookup if there's other special ones we define (meta-, alt-, possibly others)

[albers]

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

[Benehiko]

LGTM

[albers]

Let me know when I can squash the commits into a more relevant set of commits.

[laurazard]

Thank you so much for this @albers!

I'm thinking we could squash 4378ab4 (#5580) together with all the "disable file completions for X" commit, and also take care of 33d3246 (#5580) – and whatever else there is – but overall looks good!

[albers]

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

[laurazard]

Re-LGTM. Thanks!

[thaJeztah]

LGTM!

Sorry thought I already did 🙈 🙈 🙈