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

Completion support for zsh #1012

Closed
GeertJohan opened this issue Sep 21, 2018 · 2 comments
Closed

Completion support for zsh #1012

GeertJohan opened this issue Sep 21, 2018 · 2 comments

Comments

@GeertJohan
Copy link
Contributor

Currently, the skaffold completion subcommand ignores any arguments and always outputs bash completion. Since skaffold completion zsh didn't error, I expected it to return a valid zsh completion script only to find out it was running the bash completion script.

I thinks two seperate PR's can be created to fix this issue.

The first is very simple and just manages expectation. The shell argument to skaffold completion SHELL should be validated; if it's not "bash" then write an error and exit with non-zero exitcode. If maintainers agree with this I would like to create a PR for it. It's simple enough to make it in the next release.

The second PR would actually add zsh completion.
I've seen cobra supports very rudimentary Zsh now, just command structure. I guess for that reason both kubectl and minikube use a converter/wrapper from bash to zsh.

kubectl: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/completion.go
minikube: https://github.com/kubernetes/minikube/blob/master/cmd/minikube/cmd/completion.go

They both seem very similar. Probably we could just diff the files and pick one based on which is best for skaffold.

@r2d4
Copy link
Contributor

r2d4 commented Sep 21, 2018

Agreed. Both the kubectl and minikube zsh completion should be same (except s/minikube/kubectl/) and we can just copy that over. We would absolutely be open to a PR for either, or both.

When I wrote the minikube completion, I used kubectl completion as the reference, and when I wrote the skaffold completion, I used minikube as the reference :)

@dgageot
Copy link
Contributor

dgageot commented Sep 25, 2018

@GeertJohan Would you like to contribute this feature?

varunkashyap added a commit to varunkashyap/skaffold that referenced this issue Oct 26, 2018
We currently only support bash completion scripts, however as
raised in part 1 of GoogleContainerTools#1012 this means we output bash completion
script irrespective what the user types as the shell.

This PR makes bash explicit, introduces RunE instead of Run to
propagate error is unsupported shell is entered by the user.
varunkashyap added a commit to varunkashyap/skaffold that referenced this issue Oct 26, 2018
We currently only support bash completion scripts, however as
raised as part 1 of GoogleContainerTools#1012 this means we output bash completion
script irrespective what the user types as the shell.

This PR makes bash explicit, introduces valid args and checks for
length and presence of valid args
@dgageot dgageot assigned dgageot and unassigned dgageot Jan 20, 2019
corneliusweig pushed a commit to corneliusweig/skaffold that referenced this issue Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants