Set stdout so CLI completion feature works correctly #2262
Conversation
lynnemorrison
requested review from
c-kruse,
fgiorgetti and
nluaces
as code owners
| completionCommands := []string{"completion", cobra.ShellCompRequestCmd, cobra.ShellCompNoDescRequestCmd} | ||
| if len(os.Args) <= 2 || !slices.Contains(completionCommands, os.Args[1]) { | ||
| rootCmd.SetOut(os.Stderr) | ||
| } |
There was a problem hiding this comment.
For my education, could you explain what is going on here? I'm not sure I follow the logic of setting the command output stream to stderr except for completion commands.
There was a problem hiding this comment.
This change is to fix the issue that Danilo found with completion command.
skupper completion subcommand throws errors into stdout #2135
lynnemorrison
force-pushed
the
v2_cli_compl.etion
branch
from
e66094e to
384e0c5
Compare
|
I am not sure on the benefit of changing exclusively the output of If I understand the original issue correctly, the problem reported says that on an invalid usage, the help Maybe instead of just Thoughts? |
There was a problem hiding this comment.
The command still does not say what's wrong with the call, or report its error status.
I understand, however, that fixing these might take a fairly big rewrite that may not be worth it.
That said, I agree with Fernando's review, that it would be great to handle also bad arguments (and not just missing arguments). And the panic scenario needs addressed.
| rootCmd.SetHelpCommand(&cobra.Command{Hidden: true}) | ||
|
|
||
| completionCommands := []string{"completion", cobra.ShellCompRequestCmd, cobra.ShellCompNoDescRequestCmd} | ||
| if slices.Contains(completionCommands, os.Args[1]) && len(os.Args) <= 2 { |
There was a problem hiding this comment.
This will panic if no arguments are given (there's no os.Args[1], then)
4 participants
fixes 2135