-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Simplify zsh completion code #11329
Simplify zsh completion code #11329
Conversation
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
Hi @baryluk. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Welcome @baryluk! |
Can one of the admins verify this patch? |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: baryluk The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Zsh is supported by Cobra for few years now.
@baryluk thank you for this PR, do u mind pasting in the PR description the output Before and After this PR , I dont use zshell and there is no way for me to verify this, I would love to see ouput before and after this PR to help me review this PR agian thank you |
@baryluk: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@baryluk : I get an error when testing this: % ./out/minikube completion zsh | tee "${fpath[1]}/_minikube"
tee: /usr/local/share/zsh/site-functions/_minikube: Permission denied You probably want to add a |
Yes. Probably (I used sudo). I don't see any standard user-accessible location in zsh that is enabled by default. :/ |
You can install it anywhere, but apparently you need to add it to
Here is what Docker is doing: ( |
In general we don't have any instructions for doing a user-level (non-admin) installation of minikube.
Seems like these instructions were broken from before:
So it is OK to use root also for installation of the completion, like It would be better if zsh installed to $ minikube completion bash | sudo tee /etc/bash_completion.d/minikube # for bash users
$ minikube completion zsh | sudo tee "${fpath[1]}/_minikube" # for zsh users Weird that it doesn't include any configuration paths by default, neither system nor user ? /usr/local/share/zsh/site-functions |
I will go back to rebasing this code soon. Thanks for the initial checks. |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
Could we get a static copy of a working zsh completion file until this is working? |
@baryluk sorry for the delay in PR review, would u plz re-base your PR and also paste the output Before/After this PR ? |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Zsh is supported by Cobra for few years now.
No need for hacks (converting bash script to zsh).
The cobra's zsh completion relays on dynamically running minikube with
__complete
to learn completions. In my testing it is plenty fast (can't observe any delay, and manually running./out/minikube __complete ...
for various combinations, it always finishes in less than 50ms).I also manually verified that it works in zsh, and essentially all tested sub-commands and
--options
behave same way as inbash
completions. There might be minor semantic differences (I couldn't find any tho), but it should be inconsequential, not guaranteed to be stable.Also update the help text a little to be both more universal and "correct".