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

Implement kubectl custom command-line completions using go instead of bash #882

Closed
brianpursley opened this issue Jun 13, 2020 · 32 comments · Fixed by kubernetes/kubernetes#96087
Assignees
Labels
area/kubectl kind/feature Categorizes issue or PR as related to a new feature. priority/P3 sig/cli Categorizes an issue or PR as relevant to SIG CLI.

Comments

@brianpursley
Copy link
Member

brianpursley commented Jun 13, 2020

What would you like to be added:

  • Implement dynamic completions (i.e. show list of pods) using go instead of bash using new capabilities available in Cobra 1.0.0.
  • Remove all existing custom command-line completion bash and zsh workaround code and let cobra generate the completion script.
  • Add unit tests for each of the completion types (node, context, cluster, resource, etc)
  • There should be no impact to how completions are used or how they are installed, so this change should be transparent to the end-user.

Why is this needed:
The Cobra reference was recently upgraded to version 1.0.0 for the Kubernetes 1.19 release. This version of Cobra has a new, recommended way to specify dynamic completions using Go.

Note: Custom Completions written in Go will automatically work for other shell-completion scripts (e.g., Fish shell), while Custom Completions written in Bash will only work for Bash shell-completion. It is therefore recommended to use Custom Completions written in Go.

By implementing completions in Go (instead of bash), we can get rid of the custom bash code to handle completions which doesn't have tests, and we can also get rid of the zsh workaround code because Cobra 1.0.0 supports zsh completions now.

Making this change will also enable kubectl to support fish completion, which has been requested, as Cobra 1.0.0 supports fish completions.

Writing completions in go could also make it easier to optimize completions to handle large resource lists which have been found to be slow in some cases because we will have more control over how the requests are being performed internally.

@brianpursley brianpursley added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 13, 2020
@brianpursley
Copy link
Member Author

I was looking at this, and I think it will work fine. I'm going to make an attempt at this and submit a PR for review.

/assign

@marckhouzam
Copy link
Member

  • Implement dynamic completions (i.e. show list of pods) using go instead of bash using new capabilities available in Cobra 1.0.0.

Yeah!
FYI, Helm just moved to Go dynamic completions. For example:
https://github.com/helm/helm/blob/a28d695c43ad5b889923c5cf0e7694503bdb47df/cmd/helm/status.go#L56

By implementing completions in Go (instead of bash) [...] we can also get rid of the zsh workaround code because Cobra 1.0.0 supports zsh completions now.

Actually, Cobra 1.0's support for zsh completion does not yet support dynamic completions in any form. It first needs spf13/cobra#1070 and then Cobra's native zsh completions will be ready. Until then, kubectl's current zsh support is better than what Cobra supports, so should not be changed.

@brianpursley
Copy link
Member Author

@marckhouzam thanks for the reminder... I think you mentioned this before in another issue but it slipped my mind for some reason, I’m not sure why.

I think this issue is still probably something to be done, but timing wise, maybe It is best to hold off until spf13/cobra#1070 gets merged and we can upgrade to a version of cobra that supports dynamic completions for all... bash, zsh, and fish.

I don’t want to halfway implement something or end up with multiple types of completion code for different shells.

@brianpursley brianpursley removed their assignment Jun 15, 2020
@marckhouzam
Copy link
Member

I think this issue is still probably something to be done, but timing wise, maybe It is best to hold off until spf13/cobra#1070 gets merged and we can upgrade to a version of cobra that supports dynamic completions for all... bash, zsh, and fish.

@brianpursley it's really up to you but to be clearer, if you switch to dynamic completion in Go, it will work for all three shells right away, even with the current zsh workaround (that is what helm has right now: using ValidArgsFunction and the zsh workaround). The zsh workaround uses the bash completion script and that script uses dynamic completion in Go. It is Cobra 1.0's native zsh script that doesn't support custom completion at all.

So, if you have the time, I think it would be worth starting to migrate to dynamic completions in Go gradually right away. That way, once spf13/cobra#1070 is available, kubectl will be ready. And I say gradually because dynamic completion in Go can be used at the same time as dynamic completion in bash (in case the doc wasn't clear), so you can change one command at a time if you so wish.

@seans3
Copy link
Contributor

seans3 commented Jun 24, 2020

/sig cli
/area kubectl

@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. area/kubectl labels Jun 24, 2020
@eddiezane
Copy link
Member

/kind feature
/priority P3

@knight42
Copy link
Member

@brianpursley Hi! now that spf13/cobra#1070 has been merged, are you still working on this issue? If not, I am willing to work on it.

@brianpursley
Copy link
Member Author

@knight42 I didn't realize it got merged. I am not working on it currently, so if you are willing, please go ahead. I'll be happy to help if you have any questions or review the PR when you are ready.

@brianpursley
Copy link
Member Author

brianpursley commented Jul 17, 2020

@knight42 Here is a branch I was doing some work on originally, in case it helps or gives you some ideas. Feel free to use anything from it or not. It is a little old now, so some of it might not even be valid anymore
kubernetes/kubernetes@master...brianpursley:kubectl-882

@knight42
Copy link
Member

@brianpursley wow that is helpful! really appreciate it!

@brianpursley
Copy link
Member Author

@knight42 you're welcome. Like I said, take it or leave it. Don't feel obligated to use it, but if it helps as a reference then great! 😄

@knight42
Copy link
Member

/assign

@brianpursley
Copy link
Member Author

@knight42 This issue came up in today's SIG-CLI bug scrub meeting.

@pwittrock mentioned that kustomize uses posener complete to do its completions and that it might be worth considering using it for kubectl.

@eddiezane added it as a future topic for the next SIG-CLI meeting (next Wednesday).

@knight42
Copy link
Member

@brianpursley posener complete seems to be a good alternative, thanks for letting me know!

a future topic for the next SIG-CLI meeting

I am afraid I am unable to attend the meeting as the meeting time is at midnight in my TZ, but I will check the recording next day.

@marckhouzam
Copy link
Member

I haven't looked in detail at https://github.com/posener/complete but out of curiosity, if you are to rewrite the kubectl custom completions in Go either way, what's the advantage of doing it in https://github.com/posener/complete instead of directly in Cobra?

(Disclaimer: I have a bias since I developed the Cobra solution and have moved the Helm project to use it)

@knight42
Copy link
Member

knight42 commented Jul 29, 2020

@marckhouzam Hi! I am rewriting kubectl custom completions using cobra now, I wonder how could I gradually rewrite it without changing the current completion script? Basically I don't want to refactor the whole custom completion mechanism and then file a huge PR. Do you have any suggestion? Thanks in advance!

@marckhouzam
Copy link
Member

@knight42 I think you are taking the best approach by changing things gradually.

You can start by adding ValidArgsFunction or RegisterFlagCompletionFunc() to one command or flag at a time; it will not break the other custom completions still written in bash. Once you've added a completion in Go you can remove the corresponding bash completion code if no other completion is using it.

For example, you could add RegisterFlagCompletionFunc() for the --namespace flag. Here is how I did it for Helm, although for kubectl there may be some better implementation:
https://github.com/helm/helm/blob/8e11f15659cd45ccd53685bc856062d629cc413d/cmd/helm/root.go#L83-L102

Now that --namespace has its completion in Go, you can remove the corresponding bash code:
https://github.com/kubernetes/kubernetes/blob/8e8b6a01cf6bf55dea5e2e4f554597a95c82988a/pkg/kubectl/cmd/cmd.go#L294
and
https://github.com/kubernetes/kubernetes/blob/8e8b6a01cf6bf55dea5e2e4f554597a95c82988a/pkg/kubectl/cmd/cmd.go#L169-L172 (since no other completion is using this function)

I hope this helps.

@brianpursley
Copy link
Member Author

brianpursley commented Jul 29, 2020

@knight42 I have an update for you. We discussed this topic in today's SIG-CLI meeting and agreed it makes sense to proceed with using Cobra 1.0.0 because we already have a dependency on cobra, other projects (like helm) are using it, and people already have completions set up this way in their .bashrc, .zshrc, etc.

If you find that Cobra's completion is not able to let us eliminate all the hard-coded bash we have in kubectl and eliminate the zsh-specific workaround, then we can look at posener completion as an alternative option.

@knight42
Copy link
Member

@marckhouzam Really appreciate your detailed guidance, it is really helpful!

@brianpursley Thanks for letting me know, I would file a PR with experimental changes ASAP so that we could discuss on it.

@brianpursley
Copy link
Member Author

So I see spf13/cobra#1070 got merged, but it doesn't look like it has been released yet.

Because zsh completions have issues without spf13/cobra#1070, I think we are technically blocked on this until the next cobra release.

@knight42
Copy link
Member

@brianpursley As @maciaszczykm said in kubernetes/kubernetes#93714 (comment):

The changes depend on spf13/cobra#1070, but it was not included in cobra 1.0.0.

Actually, spf13/cobra#1070 is not required for Go custom completions to work for both bash and zsh. Those new custom completions will work for both bash and zsh with Cobra 1.0. And even though zsh completion for kubectl currently re-uses the bash completion script, it will continue to work just fine with Go custom completions. In fact, in Helm, we are using Cobra 1.0 and still using the zsh workaround used by kubectl, until the next release of Cobra is available.

looks like we don't have to wait for the new release of cobra.

@brianpursley
Copy link
Member Author

@brianpursley As @maciaszczykm said in kubernetes/kubernetes#93714 (comment):

The changes depend on spf13/cobra#1070, but it was not included in cobra 1.0.0.

Actually, spf13/cobra#1070 is not required for Go custom completions to work for both bash and zsh. Those new custom completions will work for both bash and zsh with Cobra 1.0. And even though zsh completion for kubectl currently re-uses the bash completion script, it will continue to work just fine with Go custom completions. In fact, in Helm, we are using Cobra 1.0 and still using the zsh workaround used by kubectl, until the next release of Cobra is available.

looks like we don't have to wait for the new release of cobra.

I don't know about everyone else, but I would not be satisfied if we still had to have to have custom scripts to make zsh work.

When I created this issue, I had hopes that this would simplify completions (by eliminate custom scripts) and make everything consistent while at the same time enabling fish and powershell completions. But it is clear now that it is not that simple.

If cobra 1.0.0 can't fully support zsh, then I see two options:

  1. Wait for cobra 1.0.0 to support zsh (really support it)
  2. Look at using Posener Complete which says it supports bash, zsh, and fish.

@marckhouzam
Copy link
Member

I don't know about everyone else, but I would not be satisfied if we still had to have to have custom scripts to make zsh work.
When I created this issue, I had hopes that this would simplify completions (by eliminate custom scripts) and make everything consistent while at the same time enabling fish and powershell completions. But it is clear now that it is not that simple.

I feel your pain. 😞
However, we would only need to keep the zsh modifications temporarily. I mean, Cobra will do a release eventually 😄
I'll mention to the maintainers that kubectl is waiting on it to try to put more pressure.

That being said, before using native zsh completion, we need to get rid of all existing custom bash scripts. Sadly until that is done, a new release of Cobra will not help (switching to native zsh completion before having moved all custom scripts to Go would cause a loss of functionality for kubectl zsh completion).

I believe the first PR on that path is ready: kubernetes/kubernetes#93714
And I believe @knight42 is waiting for it to be merged before investing more time implementing the next steps.
If merged as is, it will work for both bash and zsh, so there will be no loss of functionality, but we'd be closer to our final goal.

For helm, I've gotten rid of all custom bash scripts. That alone makes it easier to improve the completion logic and add extra features (for example, checking flags becomes really easy, so I was able to make some completions smarter). And it is a heck of a lot easier to maintain. But I am also anxiously waiting for the next Cobra release, as Helm is ready to switch to native zsh completion.

Also, with the current Cobra 1.0.0 you can start using Fish completion right away. It won't have the custom completions, but it will have all other completions of commands and flags.
I believe kubernetes/kubernetes#92989 is ready for that.
And each time parts of the custom scripts are replaced, it will automatically start working for Fish. For example, if kubernetes/kubernetes#93714 was to be merged, then fish completion would get custom flag completion immediately.

Also, Cobra is about to get full PowerShell completion support, which will also get custom completions that have been migrated to Go. See spf13/cobra#1208.

If cobra 1.0.0 can't fully support zsh, then I see two options:

  1. Wait for cobra 1.0.0 to support zsh (really support it)
  2. Look at using Posener Complete which says it supports bash, zsh, and fish.

Whichever solution is selected, the custom bash scripts must be replaced, so there is no getting away from doing that work. So I think continuing forward to get rid of all the custom scripts is a valuable step.

What do you think?

@marckhouzam
Copy link
Member

The Cobra maintainers have agreed that a release is needed. A few PRs will be included in the next few days (at least that is my understanding) and then the release will be made.

@marckhouzam
Copy link
Member

Cobra 1.1.0 has just been released 😄
https://github.com/spf13/cobra/releases/tag/v1.1.0

But we should not move to Cobra's zsh completion until we have removed all custom completion bash scripts from kubectl.

@eddiezane
Copy link
Member

eddiezane commented Oct 14, 2020

@marckhouzam thanks for pushing that through with the cobra maintainers! I've opened a PR to update our dependency.

kubernetes/kubernetes#95571

@marckhouzam
Copy link
Member

marckhouzam commented Nov 1, 2020

PR kubernetes/kubernetes#96087 moves all bash custom completions to Go. As planned by @brianpursley, it will allow to move to native zsh completion and get rid of all the zsh script code. However, we need to move to Cobra 1.1.1 for that, which is done by kubernetes/kubernetes#95571

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 31, 2021
@marckhouzam
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 4, 2021
@rikatz
Copy link
Contributor

rikatz commented Mar 22, 2021

/lifecycle active

@k8s-ci-robot k8s-ci-robot added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Mar 22, 2021
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. and removed lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. labels Jun 20, 2021
@eddiezane
Copy link
Member

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubectl kind/feature Categorizes issue or PR as related to a new feature. priority/P3 sig/cli Categorizes an issue or PR as relevant to SIG CLI.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants