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

Extra output and errors during shell completion #583

Closed
marckhouzam opened this issue Aug 19, 2022 · 6 comments · Fixed by #592
Closed

Extra output and errors during shell completion #583

marckhouzam opened this issue Aug 19, 2022 · 6 comments · Fixed by #592
Assignees
Labels
bug This issue describes a defect or unexpected behavior carvel accepted This issue should be considered for future work and that the triage process has been completed priority/important-soon Must be staffed and worked on currently or soon.

Comments

@marckhouzam
Copy link

marckhouzam commented Aug 19, 2022

What steps did you take:
There is extra output in the shell completion choices and in some cases is even causes errors during shell completion.

What happened:

For bash, notice the error in this particular case (some other bash completions do work):

bash
bash-5.1$ source <(kapp completion bash)
bash-5.1$ kapp help <TAB>
bash: ShellCompDirectiveNoFileComp

Succeeded: syntax error in expression (error token is "Succeeded")

For zsh, notice the extra output at the end (:4, Succeeded, etc):

$ source <(kapp completion zsh)
$ compdef _kapp kapp
$ kapp <TAB>
app-change                                                     -- App change (garbage-collect, list)
app-group                                                      -- app-group will deploy/delete an application for each subdirectory within a
completion                                                     -- Output shell completion code for the specified shell (bash, zsh or fish)
config-map                                                     -- Config map (list)
delete                                                         -- Delete app
deploy                                                         -- Deploy app
deploy-config                                                  -- Show default deploy config
help                                                           -- Help about any command
inspect                                                        -- Inspect app
label                                                          -- Print specified app label
list                                                           -- List all apps in a namespace
logs                                                           -- Print app's Pod logs
rename                                                         -- Rename app
service-account                                                -- Service account (list)
tools                                                          -- Tools (diff, inspect, list-labels)
version                                                        -- Print client version
:4                                                             Succeeded
Completion ended with directive: ShellCompDirectiveNoFileComp

Also happens for fish shell.

What did you expect:

Shell completion to work as expected, without error or extra output.

Anything else you would like to add:

Shell completion uses the hidden __complete command provided by the Cobra project to obtain the shell completion choices.

  1. kapp should not print Succeeded for that command
  2. output printed to stderr by the __complete command should not be redirected by kapp to stdout (that is the problem for the string output Completion ended with directive: ShellCompDirectiveNoFileComp)

This command is added by Cobra when rootCmd.Execute() is called, which makes it a little trickier to adjust the behaviour for it.

Environment:

  • kapp version (use kapp --version): 0.52.0
  • OS (e.g. from /etc/os-release): MacOS
  • Kubernetes version (use kubectl version): N/A

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@marckhouzam marckhouzam added bug This issue describes a defect or unexpected behavior carvel triage This issue has not yet been reviewed for validity labels Aug 19, 2022
@carvel-bot carvel-bot moved this to To Triage in Carvel Aug 19, 2022
@renuy
Copy link
Contributor

renuy commented Aug 19, 2022

Thanks for reporting this @marckhouzam. Checking it out.

@marckhouzam
Copy link
Author

marckhouzam commented Aug 19, 2022

BTW, some projects explicitly check if processing with the __complete command and adapt their behaviour accordingly.

For example: https://github.com/kubernetes/kubernetes/blob/d581cc90adba6c84919738841fe3e07302d53e33/staging/src/k8s.io/kubectl/pkg/cmd/cmd.go#L131

@cppforlife
Copy link
Contributor

@marckhouzam which version of kapp are you using? i believe latest of kapp should not have this problem.

@marckhouzam
Copy link
Author

@marckhouzam which version of kapp are you using? i believe latest of kapp should not have this problem.

Version 0.52.0 as installed by brew.
I just tried on the develop branch and I see the problem.

Note that this similar but not the same problem as #447. The completion command had a similar problem and now we need to fix the __complete command.

@renuy renuy added carvel accepted This issue should be considered for future work and that the triage process has been completed priority/important-soon Must be staffed and worked on currently or soon. and removed carvel triage This issue has not yet been reviewed for validity labels Aug 22, 2022
@praveenrewar
Copy link
Member

output printed to stderr by the __complete command should not be redirected by kapp to stdout (that is the problem for the string output Completion ended with directive: ShellCompDirectiveNoFileComp)

💯

@renuy renuy moved this from To Triage to Needs Review in Carvel Sep 5, 2022
Repository owner moved this from Needs Review to Closed in Carvel Sep 13, 2022
@renuy
Copy link
Contributor

renuy commented Sep 20, 2022

Released as part of v0.53.0

@github-actions github-actions bot added the carvel triage This issue has not yet been reviewed for validity label Sep 20, 2022
@praveenrewar praveenrewar removed the carvel triage This issue has not yet been reviewed for validity label Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes a defect or unexpected behavior carvel accepted This issue should be considered for future work and that the triage process has been completed priority/important-soon Must be staffed and worked on currently or soon.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants