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

Shell completion for plugins #105867

Merged
merged 3 commits into from Oct 25, 2022

Conversation

marckhouzam
Copy link
Member

@marckhouzam marckhouzam commented Oct 25, 2021

What type of PR is this?

/kind feature
/sig cli

What this PR does / why we need it:

This PR does three things:
1- includes plugin names during kubectl shell completion (cherry-pick of @superbrothers's commit from #76561)
2- allows a plugin to provide completions through an executable script named kubectl_complete-<pluginName> present on $PATH
3- adds support for shell completion to the sample-cli-plugin

Which issue(s) this PR fixes:

Fixes #74178

Special notes for your reviewer:

Tests are missing at the moment.

When doing completion for arguments or flags for a plugin, kubectl will call kubectl_complete-<plugin> to obtain the list of completions. For example, for the krew plugin, when the user triggers completion by doing:

kubectl krew <TAB>

kubectl will look on $PATH for an executable file called kubectl_complete-krew. This file should print the list of valid completions for the plugin to stdout. kubectl will then present these completion choices to the user. This approach is the one used by the Helm project for its plugins: https://helm.sh/docs/topics/plugins/#dynamic-completion

Backwards-compatibility:

The solution is backwards-compatible with plugins in both cases:

  1. existing plugins will work with a new kubectl using this solution
  2. plugins supporting this solution will work with an old kubectl

Krew:

From what I can see from krew (I'm no expert), there is no current way to deliver a separate executable kubectl_complete-<plugin> completion file. Therefore, if this solution is chosen, it may be interesting to add support for it in krew.

Explanation: krew installs one plugin binary on $PATH using the plugin naming convention. That means that the only way for a plugin completion solution to work out-of-the-box with krew would be to call the binary plugin itself to get completions (using a special sub-command or flag). This is not a viable solution because if a plugin ignores arguments and flags, calling it to obtain completions will mistakenly run the actual plugin logic.

How to test

Manually creating a completion script:

For zsh (but very similar for bash, fish or powershell).

We need a plugin to provide completion through a new kubectl_complete-<pluginName> file. Let's create one ourselves. Let's do it for the krew plugin, which I assume many in the community have installed. Since krew uses Cobra, it has the __complete command, which makes it very easy to obtain completions, as will become clear below:

cat <<EOF >kubectl_complete-krew
#!/usr/bin/env sh

# Call the __complete command passing it all arguments
kubectl krew __complete "\$@"
EOF

chmod u+x kubectl_complete-krew

Please put the kubectl_complete-krew file somewhere on your $PATH. Then:

cd $GOPATH/src/k8s.io/kubernetes
go build -o /tmp/kubectl ./cmd/kubectl
source <(/tmp/kubectl completion zsh)

/tmp/kubectl <TAB>
# Notice that plugins are now listed as choices

/tmp/kubectl krew <TAB>
# Notice that krew sub-commands are now listed

Automatically generating completion scripts:

I've written a new plugin, "plugin-completion", which automatically generates the proper shell completion script for installed plugins that use Cobra. It works with more than half the plugins registered with krew. It can help test this PR quite easily.

NOTE: Flag completion for plugins works with Cobra 1.3 which is now part of the master branch.

Does this PR introduce a user-facing change?

Shell completion will now show plugin names when appropriate.  Furthermore, shell completion will work for plugins that provide such support.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. sig/cli Categorizes an issue or PR as relevant to SIG CLI. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 25, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @marckhouzam. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 25, 2021
@chmouel
Copy link
Contributor

chmouel commented Oct 25, 2021

Thanks for taking this forward,

is there any reasons we would not be able to use a conventional flag across plugins, i.e: kubectl-<plugin-name> --internal-completion ? I am just worried if there is a bunch of kubectl_complete-<plugin-name> in path, users may get confuse on what it does when using the binary directly.

@marckhouzam
Copy link
Member Author

is there any reasons we would not be able to use a conventional flag across plugins, i.e: kubectl-<plugin-name> --internal-completion ?

I agree such an approach would be a more elegant solution. It was discussed in #74178 early on but then we realized that it would create a backwards-compatibility issue. If a plugin does not support shell completion but kubectl still calls it with a flag (e.g., --internal-completion), it could have unexpected side-effects; for instance, the plugin may very well ignore that flag and execute its normal functionality.

So we need a solution that also indicates to kubectl if the plugin does support completion or not. A completely separate kubectl_complete-<plugin> file achieves this by its simple presence or absence.

I am just worried if there is a bunch of kubectl_complete-<plugin-name> in path, users may get confuse on what it does when using the binary directly.

I agree there too.

We could hide the files using a dot prefix: .kubectl_complete-<plugin>. Not sure how much that helps.

We could instead use an agreed upon location that is not on PATH, e.g., $HOME/.kubectl-plugins/ where plugins would need to install the kubectl_complete-<plugin> script. Having such a location would also be useful if we choose to implement phase 2 which needs to store a yaml file somewhere.

For this draft I mimicked the current plugin approach for simplicity, but I'd appreciate some guidance on what would be the best approach.

@eddiezane
Copy link
Member

@marccampbell I'm sure y'all will have some thoughts here.

@soltysh
Copy link
Contributor

soltysh commented Oct 28, 2021

/assign

@Drugoy
Copy link

Drugoy commented Nov 7, 2021

@marckhouzam Hey Marc!
I'm a bit confused about how your suggested solution works.
Did I get it right that in order to bring shell completion for my self-written kubectl plugin I would need to manually rebuild kubectl with a patch that adds some instructions on how kubectl should provide completion for my plugin?

Also, in your example - I don't see what list of values would be suggested as completions for your plugin, could you explain how it'd work?

@marckhouzam
Copy link
Member Author

marckhouzam commented Nov 7, 2021

@marckhouzam Hey Marc! I'm a bit confused about how your suggested solution works. Did I get it right that in order to bring shell completion for my self-written kubectl plugin I would need to manually rebuild kubectl with a patch that adds some instructions on how kubectl should provide completion for my plugin?

Hi @Drugoy. Currently, if you want to try the proposed solution, then you need to rebuild kubectl using this PR and then you need to provide an executable file called kubectl_complete-<yourplugin> which can be found on $PATH. kubectl will call this file passing it the current command-line following your plugin name and will expect the file to output the correct completions for the specified command-line.

So if you take the krew plugin for example, and the user types kubectl krew u<TAB>, kubectl built with this PR will try to find an executable called kubectl_complete-krew and will call it passing it the u argument. That executable file can look something like this:

#!/bin/bash

# Krew has the following sub-commands
echo update
echo upgrade
echo search
echo install
echo uninstall

This is a simple example, but you would have to also handle flags if the user typed a -, but also output different completions if the user has already specified the first level of subcommands on the command-line, etc

Maybe the helm documentation can help? I'm proposing to do the same thing for kubectl plugins: https://helm.sh/docs/topics/plugins/#providing-shell-auto-completion

@Drugoy
Copy link

Drugoy commented Nov 7, 2021

@marckhouzam, thanks for explaining in details.
But what about dynamic lists that are currently being provided by kubectl completion functions?
How to reuse them for plugins?

2 simple kubectl plugin example ideas: say, I don't like to remember how to use kubectl to do a few things, so, instead, I'd like to call:

  • kubectl context ${context} to switch active context (i.e. that "plugin" is an alias for kubectl config use-context ${context} and ${context} should be tab-completable for the plugin)
  • kubectl namespace ${namespace} to switch namespace for the active context (i.e. that "plugin" is an alias for kubectl config set-context --current --namespace=${namespace} and ${namespace} should be tab-completable for the plugin)

In the first example kubectl parses kube-config to list existing contexts.
In the second example kubectl queries KubeAPI (of the Kubernetes cluster from active context) to list some resources like pods/deployments/namespaces/whatever.
In both examples the lists are generated/dynamic and instead of writing code do what kubectl does to show these lists - I'd like to be able to just reference kubectl's existing completion functions.
What do you think about that?

p.s.: IMO such completion cases should be treated like specials and there shouldn't be necessity to provide any extra kubectl_complete-* files, but instead there should be a way to just reference kubectl's completion functions somehow.

@marckhouzam
Copy link
Member Author

@marckhouzam, thanks for explaining in details. But what about dynamic lists that are currently being provided by kubectl completion functions? How to reuse them for plugins?

  • kubectl namespace ${namespace} to switch namespace for the active context (i.e. that "plugin" is an alias for kubectl config set-context --current --namespace=${namespace} and ${namespace} should be tab-completable for the plugin)

For dynamic completions your kubectl_complete-<plugin> file can call kubectl (or any other program) to obtain the list of completions. For example, in your example above, the kubectl_complete-namespace script can be (I may not have gotten the syntax perfectly but it illustrates the idea):

#!/bin/bash
kubectl get namespaces -o name

Such a script will provide all namespace names as completions for that plugin.

p.s.: IMO such completion cases should be treated like specials and there shouldn't be necessity to provide any extra kubectl_complete-* files, but instead there should be a way to just reference kubectl's completion functions somehow.

Where would you reference such completions function? You need the kubectl_complete-* to specify that reference 🙂

@Drugoy
Copy link

Drugoy commented Nov 8, 2021

@marckhouzam
IMO it shouldn't be scripts in PATH envvar, but rather be shell functions, just like kubectl currently works.
Currently, to 'install' kubectl completions one needs to do something like source <(kubectl completion "$(basename "${SHELL}")" and that registers _kubectl function that is used for completions.
You can inspect that function's code via which _kubectl.

IMO, kubectl plugins completions should follow this approach and also use functions.
As far as I understand, shell doesn't really care much if a command you call is a function call or an execution of some binary (from PATH), so all that probably needs to be changed here is the name of the binary/function: IMO, just like _kubectl - completions for kubectl plugins should also start with an underscore.

As for your bash scripts illustrating how to provide completions for kubectl plugins - it looks like the only requisite for completion scripts/functions is that they return a list of items, where each item should be on its own line.
If that's the case, your last example is a bit incorrect, as it returns namespaces with namespace/ static prefix.
kubectl get namespaces --no-headers -o custom-columns="NAME:.metadata.name" or kubectl get namespaces -ojsonpath='{range .items[*]}{.metadata.name}{"\n"}{end} should be used instead.

But that was an illustration that provided only a single completion list, while kubectl has multiple completion lists: some sub-commands have arguments (like -n/--namespace) that have their own completion lists!

What'd you suggest to do here?
To have a bunch of separate scripts in PATH for that?
How would that look like if my plugin would need to have -n {TAB} completion that should suggest namespaces and final arg completion that should suggest pods (from the namespace specified in -n CLI arg or from active namespace if -n arg was not provided)?

@marckhouzam
Copy link
Member Author

@marckhouzam IMO it shouldn't be scripts in PATH envvar, but rather be shell functions, just like kubectl currently works.
IMO, kubectl plugins completions should follow this approach and also use functions.

This is an interesting point. IIUC, with such an approach, for a plugin named namespace, kubectl would call the shell function _kubectl-namespace to obtain the completions? Is that what you mean? In such a case however, how will the plugin tell kubectl which completion function to call and when? Also, I assume we need to implement these functions in each of the four shell languages supported by kubectl (bash, zsh, fish, powershell)?

But that was an illustration that provided only a single completion list, while kubectl has multiple completion lists: some sub-commands have arguments (like -n/--namespace) that have their own completion lists!

What'd you suggest to do here? To have a bunch of separate scripts in PATH for that? How would that look like if my plugin would need to have -n {TAB} completion that should suggest namespaces and final arg completion that should suggest pods (from the namespace specified in -n CLI arg or from active namespace if -n arg was not provided)?

You would have to write a single smart script or program that would check the arguments passed to it and print the proper completions based on that. Here is an example of a helm plugin for which I wrote this completion script: https://github.com/marckhouzam/helm-fullstatus/blob/master/plugin.complete

I realize it can be quite complicated to write a proper completion script depending on the complexity of the plugin, but I don't think there is anything kubectl can do about that for dynamic completions: only the plugins knows how to find the completions. For static completions however I am proposing an improvement to the current PR as described here: https://helm.sh/docs/topics/plugins/#static-auto-completion. It should make static completions very easy to implement for a plugin.

Another way to make plugin completion very easy is to use a framework that provides support for completions. For example, plugins written in Go using the Cobra project, can provide completions much more easily; I give such an example for krew (which uses Cobra) in the description of this PR. Does this make sense?

@Drugoy
Copy link

Drugoy commented Nov 9, 2021

This is an interesting point. IIUC, with such an approach, for a plugin named namespace, kubectl would call the shell function _kubectl-namespace to obtain the completions? Is that what you mean?

Something like that, yeah.

In such a case however, how will the plugin tell kubectl which completion function to call and when?

I don't really have an explanation: I tried to understand how kubectl's shell completion works, but since shell completion plugins in general are already some kind of arcane magic to me and since kubectl seems to use a very clever (but not very maintainable, as it looks to me) approach of introducing more and more completion functions upon execution of the initial completion function - I simply can't answer your question.
Although I get your desire to make things simple - I think it'd be way better if you figured out how kubectl's own completions work and maybe then you could extend them in an elegant way.

I see that if I call _kubectl directly from interactive shell it says
_arguments:comparguments:325: can only be called from completion function
so it seems to use _arguments + _describe functions to figure out the context of completion.

Also, I assume we need to implement these functions in each of the four shell languages supported by kubectl (bash, zsh, fish, powershell)?

Well, yeah, since that's what the currently documented approach of configuring shell completions for kubectl already is:

Enable shell autocompletion
kubectl provides autocompletion support for Bash and Zsh, which can save you a lot of typing.

Below are the procedures to set up autocompletion for Bash and Zsh.
...

In the source code of kubectl I actually see instructions for fish and powershell as well.

You would have to write a single smart script or program that would check the arguments passed to it and print the proper completions based on that.
...
Does this make sense?

Maybe you are right. I see that kubectl's completions also use Cobra package, but I thought there should be an easy way to re-use them without writing much of own code.

@marckhouzam
Copy link
Member Author

I don't really have an explanation: I tried to understand how kubectl's shell completion works, but since shell completion plugins in general are already some kind of arcane magic to me

I know how you feel, it took me a very long time to understand it and I'm still learning 😉

and since kubectl seems to use a very clever (but not very maintainable, as it looks to me) approach of introducing more and more completion functions upon execution of the initial completion function

This has changed with kubectl 1.22 which introduces (through the Cobra project), the use of the __complete command to obtain completions. You can try the following for example, which will complete all namespaces starting with the letter k, but you can complete any command-line after the __complete command:

$ kubectl __complete -n k
kube-node-lease
kube-public
kube-system
:4
Completion ended with directive: ShellCompDirectiveNoFileComp

Also, I assume we need to implement these functions in each of the four shell languages supported by kubectl (bash, zsh, fish, powershell)?

Well, yeah, since that's what the currently documented approach of configuring shell completions for kubectl already is

kubectl supports different shells, but thanks to Cobra it only needs to implement the completion logic in Go, and Cobra takes care of handling the different shells. Please see details in https://github.com/spf13/cobra/blob/master/shell_completions.md#dynamic-completion-of-nouns

You would have to write a single smart script or program that would check the arguments passed to it and print the proper completions based on that.
...
Does this make sense?

Maybe you are right. I see that kubectl's completions also use Cobra package, but I thought there should be an easy way to re-use them without writing much of own code.

You are right. The way to do that is to call kubectl directly and obtain the information you need. A shell function would not be able to do any better anyway as it would also need to call kubectl to get the information.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 12, 2021
@marckhouzam
Copy link
Member Author

Alright, I've finally rebased this PR and posted a PR to update the plugin KEP to describe the proposed shell-completion solution for plugins: kubernetes/enhancements#3496

I've also added to this PR support for shell completion for the sample-cli-plugin which could serve as an example.

Hopefully we can get this in to 1.26 🤞

@marckhouzam
Copy link
Member Author

/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 Sep 6, 2022
@marckhouzam
Copy link
Member Author

/retest

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor nits here and there, but all looks great - thank you! And apologies for the long delays 😅

/lgtm
/approve

// plugin. This is only done when performing shell completion that relate
// to plugins.
func SetupPluginCompletion(cmd *cobra.Command, args []string) {
if len(args) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: if you reverse this condition, the code will be easier to read without unnecessary indentation.

if len(args) == 0 {
    return
}
// the rest of the code goes here...


var comps []string
directive := cobra.ShellCompDirectiveDefault
if err := prog.Run(); err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same nit as above, fail fast to limit the indentation 😉

if err := prog.Run(); err != nil {
    return comps, directive
}
// the rest of the code goes here...

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 25, 2022
@soltysh
Copy link
Contributor

soltysh commented Oct 25, 2022

/triage accepted
/priority backlog

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. priority/backlog Higher priority than priority/awaiting-more-evidence. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 25, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marckhouzam, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 25, 2022
@k8s-ci-robot k8s-ci-robot merged commit 797536f into kubernetes:master Oct 25, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Oct 25, 2022
@superbrothers
Copy link
Member

@marckhouzam Great work! 👏 🎉

@marckhouzam marckhouzam deleted the feature/compPlugins branch October 28, 2022 23:06
ahmedtd pushed a commit to ahmedtd/enhancements that referenced this pull request Feb 2, 2023
This proposal was discussed in
kubernetes/kubernetes#105867

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
maelvls added a commit to maelvls/kubectl-view-secret that referenced this pull request Feb 3, 2023
For this to work, you will need kubectl 1.26 and a shim that you can
create with the following command:

cat <<'EOF' >~/bin/kubectl_complete-view_secret && chmod u+x ~/bin/kubectl_complete-view_secret
kubectl view-secret __complete "$@"
EOF

See: kubernetes/kubernetes#105867
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Command line completion for kubectl plugins