Allow zsh completion to be autoloaded by compinit #50561

cblecker · 2017-08-12T20:42:35Z

What this PR does / why we need it:
Allows the kubectl zsh autocompletion to be auto loaded by compinit. Had to move the the boilerplate down into the specific shell functions as the compdef needs to be the first line in the definition file.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #50560

Special notes for your reviewer:

Release note:

kubectl zsh autocompletion will work with compinit

cblecker · 2017-08-12T20:42:57Z

/sig cli

cblecker · 2017-08-12T22:17:35Z

/retest

cblecker · 2017-08-12T22:52:38Z

@kubernetes/sig-cli-pr-reviews

sttts · 2017-08-14T09:35:12Z

@cblecker lgtm. Can you add an example to the kubectl help text how to autoload it?

cblecker · 2017-08-14T18:27:49Z

@sttts Updated. PTAL!

cblecker · 2017-08-14T20:05:44Z

/retest

fabianofranz · 2017-08-14T20:22:07Z

/assign @juanvallejo

cblecker · 2017-08-14T21:23:23Z

/retest

sttts · 2017-08-15T06:42:33Z

pkg/kubectl/cmd/completion.go

+		    brew install bash-completion@2
+		## If kubectl is installed via homebrew, this should start working immediately.
+		## If you've installed via other means, you may need add the completion to your completion directory
+		    kubectl completion bash > $(brew --prefix)/etc/bash_completion.d/kubectl


this is a bad hint. Can we update it to: echo 'source <(kubectl completion bash)' > $(brew --prefix)/etc/bash_completion.d/kubectl ?

this actually isn't a bad hint.. if you pipe the output to this file, it will automatically be loaded by bash_completion on start

ref: https://kubernetes.io/docs/tasks/tools/install-kubectl/#enabling-shell-autocompletion

sttts · 2017-08-15T06:43:13Z

pkg/kubectl/cmd/completion.go

+		## Load the kubectl completion code for bash into the current shell
+		    source <(kubectl completion bash)
+		## Write bash completion code to a file and source if from .bash_profile
+		    kubectl completion bash > ~/.kube/completion.bash.inc


same here. This is a bad hint.

This isn't a bad hint either. You're piping the output to a file, and then sourcing it. This is actually just reformatting what was already there.

We shouldn't store the output, but call source <(kubectl completion bash). Otherwise, people will complain on the next kubectl update that things break.

sttts · 2017-08-15T06:43:44Z

pkg/kubectl/cmd/completion.go

-		source <(kubectl completion zsh)`))
+		    source <(kubectl completion zsh)
+		# Set the kubectl completion code for zsh[1] to autoload on startup
+		    kubectl completion zsh > "${fpath[1]}/_kubectl"`))


This is actually the step you do to load the kubectl completion on startup in zsh. zsh looks for a compdef in it's fpath

I like this compdef approach. Just wondering whether we can combine it with source <(kubectl completion zsh), i.e. not storing the output of kubectl, but sourcing it again when zsh loads the kubectl completion. The usecase I have in mind is that of kubectl which comes either via some packaging system or via the gcloud cli. I would bet people forget to update their completion when a new kubectl is installed.

My concern is that both the bash completion FAQ and the zsh docs suggest that the preferred thing to do is to store them in a file. I agree that sourcing directly from the application keeps it up to date, but it could also be a performance issue loading the functions into memory instead of loading them as needed from files dynamically.

juanvallejo · 2017-08-15T17:23:57Z

lgtm pending @sttts 's comments

alexandercampbell

Minor comments

alexandercampbell · 2017-08-17T15:35:42Z

pkg/kubectl/cmd/completion.go

-
-		    $ source $(brew --prefix)/etc/bash_completion
+		Detailed instructions on how to do this are available here:
+		https://kubernetes.io/docs/tasks/tools/install-kubectl/#enabling-shell-autocompletion


+1 to having a single source of truth here.

alexandercampbell · 2017-08-17T15:37:17Z

pkg/kubectl/cmd/completion.go

-func runCompletionZsh(out io.Writer, kubectl *cobra.Command) error {
+func runCompletionZsh(out io.Writer, boilerPlate string, kubectl *cobra.Command) error {
+	zsh_head := `#compdef kubectl
+`


Why not replace this line with this?

zshHead := "#compdef kubectl\n"

Easier to read and won't annoy the linter.

Great suggestion. Fixed.

cblecker · 2017-08-17T18:32:32Z

/retest

cblecker · 2017-08-19T20:46:22Z

/retest

cblecker · 2017-08-23T19:19:13Z

How should we proceed on this @sttts? I agree with your concern about keeping the definition up to date, and with the compdef change of this PR both will work.. but I'm concerned about making a recommendation that does against the docs of the completion frameworks themselves. I'm unsure what the best balance of the two concerns is.

sttts · 2017-08-23T20:31:41Z

@cblecker quick benchmark: source <(kubectl completion bash) 140ms, source stored-completion 36ms. Clear winner.

Let's go forward with this. /lgtm

cblecker · 2017-08-23T20:52:11Z

Heh, yeah that's a pretty large difference. That work for you too, @juanvallejo?

juanvallejo · 2017-08-23T23:10:48Z

/lgtm

cblecker · 2017-08-24T04:34:51Z

/assign @dchen1107
for approval

cblecker · 2017-08-25T19:12:13Z

/assign @fabianofranz
for approval if you have time

fabianofranz · 2017-08-28T19:59:38Z

/approve

k8s-github-robot · 2017-08-28T19:59:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, fabianofranz, juanvallejo

Associated issue: 50560

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

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubectl/OWNERS~~ [fabianofranz]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

cblecker · 2017-08-28T21:47:49Z

/test pull-kubernetes-verify
godep download failed. trying again

cblecker · 2017-08-28T23:10:41Z

/retest

fejta-bot · 2017-08-29T02:49:46Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

cblecker · 2017-08-29T06:57:26Z

/retest

k8s-github-robot · 2017-08-29T09:22:09Z

Automatic merge from submit-queue (batch tested with PRs 51471, 50561, 50435, 51473, 51436)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 12, 2017

k8s-ci-robot added the sig/cli Categorizes an issue or PR as relevant to SIG CLI. label Aug 12, 2017

k8s-github-robot assigned dchen1107 and smarterclayton Aug 12, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 12, 2017

cblecker mentioned this pull request Aug 12, 2017

kubernetes-cli: Ensure _kubectl is auto-loaded by zsh compinit Homebrew/homebrew-core#16507

Closed

4 tasks

cblecker force-pushed the zsh-compinit branch from 4fdc075 to d197983 Compare August 14, 2017 18:27

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 14, 2017

k8s-ci-robot assigned juanvallejo Aug 14, 2017

sttts reviewed Aug 15, 2017

View reviewed changes

alexandercampbell approved these changes Aug 17, 2017

View reviewed changes

cblecker added 2 commits August 18, 2017 14:29

Allow zsh completion to be autoloaded by compinit

70b942d

Update help/example for kubectl completion

0153f8d

cblecker force-pushed the zsh-compinit branch from 495138e to 0153f8d Compare August 18, 2017 21:29

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2017

k8s-ci-robot assigned fabianofranz Aug 25, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2017

k8s-github-robot merged commit b5c5b4a into kubernetes:master Aug 29, 2017

cblecker deleted the zsh-compinit branch August 29, 2017 17:04

This was referenced Jul 15, 2018

Allow zsh completion to be autoloaded by compinit helm/helm#4338

Merged

Allow zsh completion to be autoloaded by compinit dustyrip/minikube#1

Closed

Allow zsh completion to be autoloaded by compinit kubernetes/minikube#2987

Merged

Allow zsh completion to be autoloaded by compinit #50561

Allow zsh completion to be autoloaded by compinit #50561

Conversation

cblecker commented Aug 12, 2017 • edited Loading

cblecker commented Aug 12, 2017

cblecker commented Aug 12, 2017

cblecker commented Aug 12, 2017

sttts commented Aug 14, 2017

cblecker commented Aug 14, 2017

cblecker commented Aug 14, 2017

fabianofranz commented Aug 14, 2017

cblecker commented Aug 14, 2017

sttts Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

cblecker Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Aug 15, 2017

alexandercampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cblecker commented Aug 17, 2017

cblecker commented Aug 19, 2017

cblecker commented Aug 23, 2017

sttts commented Aug 23, 2017

cblecker commented Aug 23, 2017

juanvallejo commented Aug 23, 2017

cblecker commented Aug 24, 2017

cblecker commented Aug 25, 2017

fabianofranz commented Aug 28, 2017

k8s-github-robot commented Aug 28, 2017

cblecker commented Aug 28, 2017

cblecker commented Aug 28, 2017

fejta-bot commented Aug 29, 2017

cblecker commented Aug 29, 2017

k8s-github-robot commented Aug 29, 2017

cblecker commented Aug 12, 2017 •

edited

Loading

sttts Aug 15, 2017 •

edited

Loading

cblecker Aug 15, 2017 •

edited

Loading