cli/command/container: unify completion funcs for create and run

[thaJeztah]

• relates to Improve Cobra completions for run and create #5580

Define completion funcs for create and run together with where the flags are defined.

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

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.58%. Comparing base (32ff200) to head (9454c56).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5581      +/-   ##
==========================================
- Coverage   59.60%   59.58%   -0.02%     
==========================================
  Files         345      345              
  Lines       29103    29093      -10     
==========================================
- Hits        17346    17336      -10     
  Misses      10788    10788              
  Partials      969      969              

[albers]

@thaJeztah My first attempt in #5580 was also to place the completions here, but I was not sure whether this was really approprtiate.

The completions add a lot of code that does not contribute to the main purprose of the program.
Most of the code will go untested because it is trivial, so coverage will decay - which might get developers on the fence.
So I tried to get the completion logic "out of sight". The legacy completions were out of sight as well.

On the other hand, putting the completion logic next to the flag definitions will give you a clear view of the existing completions, forcing authors to think about completion when they work on flags.
It is like documentation that should live close to the code.

It probably burns down to how much the developers value the completion code.

[thaJeztah]

On the other hand, putting the completion logic next to the flag definitions will give you a clear view of the existing completions, forcing authors to think about completion when they work on flags.

Yes, that was a bit my train of thought, as I think they should go together: when adding a flag, it should come with completion (and vice versa; when removing the flag, the completion can be removed). Putting them together could reduce risks of things like removing a flag (but completion to be left behind), and may be good as a template for others ("oh, adding a flag should also have completion!").

It probably burns down to how much the developers value the completion code.

I think it's important for sure! And, fun fact is that whenever "what's important for a good CLI" is brought up in interviews I've had (and know others had), one of the first things that come up is "good completion". From that perspective, I hope that the Cobra completion will help bring implementing proper completion more within reach of "the average engineer working on the CLI code", as now they have no "real" excuse not to add them (being able to write it in Go), plus when doing so having it available for more shells than just Bash.

Most of the code will go untested because it is trivial, so coverage will decay

I think the Cobra completion is more testable; at least, it's possible to have unit-tests for them that can be written in Go (so also would show up in test-coverage). We should probably look at more extensive tests beyond just unit-tests, but I'm already happy with some of the tests covering the basics (it's at least more coverage than we had with the ones in contrib/).

Now, there's things I don't like in the current implementation we have with Cobra;

• Our current use of suppressing errors returned by _ = cmd.RegisterFlagCompletionFunc; I want to have a look at a helper function for this; we don't need to return the error, but I'm considering a utility that can be run in our CI that would conditionally error out
• I'm considering making completion required, This is a bit contrary to my other suggestion to set NoComplete by default, but perhaps we can do without the boilerplating somehow.
• ☝️ we could have a test in CI that logs all flags that don't have completion configured (iterate over all flags, and print which ones don't have completion),

[albers]

I agree with you regarding the role of completions.

• Our current use of suppressing errors returned by _ = cmd.RegisterFlagCompletionFunc; I want to have a look at a helper function for this; we don't need to return the error, but I'm considering a utility that can be run in our CI that would conditionally error out

Kubectl has a utility function for that, see here