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

Avoid conflicts with other cobra auto completions #70470

Merged
merged 1 commit into from Jul 10, 2019

Conversation

dtaniwaki
Copy link
Contributor

@dtaniwaki dtaniwaki commented Oct 31, 2018

What type of PR is this?

/kind bug

What this PR does / why we need it:

kubectl get -n foo pod [TAB][TAB] ignores the namespace while kubectl get --namespace foo pod [TAB][TAB] and kubectl get --namespace=foo pod [TAB][TAB] don't.

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

N/A

Special notes for your reviewer:

N/A

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. kind/bug Categorizes issue or PR as related to a bug. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 31, 2018
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 31, 2018
@idealhack
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Oct 31, 2018
@idealhack
Copy link
Member

I don't think I can reproduce this bug

Client Version: version.Info{Major:"1", Minor:"12", GitVersion:"v1.12.0", GitCommit:"0ed33881dc4355495f623c6f22e7dd0b7632b7c0", GitTreeState:"clean", BuildDate:"2018-09-28T15:18:13Z", GoVersion:"go1.11", Compiler:"gc", Platform:"darwin/amd64"}

@dtaniwaki
Copy link
Contributor Author

dtaniwaki commented Oct 31, 2018

Here's my result without this fix.

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"12", GitVersion:"v1.12.2", GitCommit:"17c77c7898218073f14c8d573582e8d2313dc740", GitTreeState:"clean", BuildDate:"2018-10-30T21:39:38Z", GoVersion:"go1.11.1", Compiler:"gc", Platform:"darwin/amd64"}

$ kubectl get -n kube-system pod
my-pod-1
my-pod-2

$ kubectl get --namespace kube-system pod
coredns-99b9bb8bd-fpwfr
etcd-minikube
kube-addon-manager-minikube
kube-apiserver-minikube
kube-controller-manager-minikube
kube-proxy-mkl4h
kube-scheduler-minikube
kubernetes-dashboard-7db4dc666b-vbnhv
storage-provisioner
tiller-deploy-64c9d747bd-6r7cn

@dtaniwaki
Copy link
Contributor Author

/assign @deads2k

@dtaniwaki
Copy link
Contributor Author

@deads2k Could you review this PR?

@idealhack
Copy link
Member

cc @kubernetes/sig-cli-pr-reviews

@@ -84,7 +84,11 @@ __kubectl_override_flags()
local ${__kubectl_override_flag_list[*]##*-} two_word_of of var
for w in "${words[@]}"; do
if [ -n "${two_word_of}" ]; then
eval "${two_word_of##*-}=\"${two_word_of}=\${w}\""
if [[ "${two_word_of}" == --* ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

If that is a bug, I don't think this pr will actually fix it.

The changes here is only used for bash completion, which will do nothing for parsing arguments.

Copy link
Member

Choose a reason for hiding this comment

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

The bug is in the autocompletion… the suggestions on [tab][tab] do not honor the specified namespace

Copy link
Contributor Author

@dtaniwaki dtaniwaki Jan 12, 2019

Choose a reason for hiding this comment

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

The bug is in the autocompletion… the suggestions on [tab][tab] do not honor the specified namespace

Yes, that is what I wanted to fix, but it seems I was looking into a wrong place. I'm trying to update reproducible steps and fix the issue.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 12, 2019
@dtaniwaki
Copy link
Contributor Author

dtaniwaki commented Jan 12, 2019

I added debug logs for auto completion commands and found which patterns don't work correctly.

kubectl get pod -n argo
kubectl get pod --namespace=argo
kubectl get pod --namespace argo
kubectl get -n argo pod
kubectl get --namespace=argo pod
kubectl get --namespace argo pod

I enabled debug logs and got the following.

the completion log
__kubectl_handle_word: c is 0 words[c] is kubectl
__kubectl_handle_command: c is 0 words[c] is kubectl
__kubectl_handle_command: looking for _kubectl_root_command
__kubectl_handle_word: c is 1 words[c] is get
__kubectl_handle_command: c is 1 words[c] is get
__kubectl_handle_command: looking for _kubectl_get
__kubectl_handle_word: c is 2 words[c] is pod
__kubectl_handle_noun: c is 2 words[c] is pod
__kubectl_handle_word: c is 3 words[c] is -n
__kubectl_handle_flag: c is 3 words[c] is -n
__kubectl_handle_flag: looking for -n
__kubectl_handle_flag: found a flag -n, skip the next argument
__kubectl_handle_reply
__kubectl_parse_get: get completion by kubectl get -n=argo -o template --template="{{ range .items  }}{{ .metadata.name }} {{ end }}" "pod"
__kubectl_handle_word: c is 0 words[c] is kubectl
__kubectl_handle_command: c is 0 words[c] is kubectl
__kubectl_handle_command: looking for _kubectl_root_command
__kubectl_handle_word: c is 1 words[c] is get
__kubectl_handle_command: c is 1 words[c] is get
__kubectl_handle_command: looking for _kubectl_get
__kubectl_handle_word: c is 2 words[c] is pod
__kubectl_handle_noun: c is 2 words[c] is pod
__kubectl_handle_word: c is 3 words[c] is -n
__kubectl_handle_flag: c is 3 words[c] is -n
__kubectl_handle_flag: looking for -n
__kubectl_handle_flag: found a flag -n, skip the next argument
__kubectl_handle_reply
__kubectl_parse_get: get completion by kubectl get -n=argo -o template --template="{{ range .items  }}{{ .metadata.name }} {{ end }}" "pod"
__kubectl_handle_word: c is 0 words[c] is kubectl
__kubectl_handle_command: c is 0 words[c] is kubectl
__kubectl_handle_command: looking for _kubectl_root_command
__kubectl_handle_word: c is 1 words[c] is get
__kubectl_handle_command: c is 1 words[c] is get
__kubectl_handle_command: looking for _kubectl_get
__kubectl_handle_word: c is 2 words[c] is pod
__kubectl_handle_noun: c is 2 words[c] is pod
__kubectl_handle_word: c is 3 words[c] is --namespace=argo
__kubectl_handle_flag: c is 3 words[c] is --namespace=argo
__kubectl_handle_flag: looking for --namespace=
__kubectl_handle_reply
__kubectl_parse_get: get completion by kubectl get --namespace=argo -o template --template="{{ range .items  }}{{ .metadata.name }} {{ end }}" "pod"
__kubectl_handle_word: c is 0 words[c] is kubectl
__kubectl_handle_command: c is 0 words[c] is kubectl
__kubectl_handle_command: looking for _kubectl_root_command
__kubectl_handle_word: c is 1 words[c] is get
__kubectl_handle_command: c is 1 words[c] is get
__kubectl_handle_command: looking for _kubectl_get
__kubectl_handle_word: c is 2 words[c] is pod
__kubectl_handle_noun: c is 2 words[c] is pod
__kubectl_handle_word: c is 3 words[c] is --namespace=argo
__kubectl_handle_flag: c is 3 words[c] is --namespace=argo
__kubectl_handle_flag: looking for --namespace=
__kubectl_handle_reply
__kubectl_parse_get: get completion by kubectl get --namespace=argo -o template --template="{{ range .items  }}{{ .metadata.name }} {{ end }}" "pod"
__kubectl_handle_word: c is 0 words[c] is kubectl
__kubectl_handle_command: c is 0 words[c] is kubectl
__kubectl_handle_command: looking for _kubectl_root_command
__kubectl_handle_word: c is 1 words[c] is get
__kubectl_handle_command: c is 1 words[c] is get
__kubectl_handle_command: looking for _kubectl_get
__kubectl_handle_word: c is 2 words[c] is pod
__kubectl_handle_noun: c is 2 words[c] is pod
__kubectl_handle_word: c is 3 words[c] is --namespace
__kubectl_handle_flag: c is 3 words[c] is --namespace
__kubectl_handle_flag: looking for --namespace
__kubectl_handle_word: c is 4 words[c] is argo
__kubectl_handle_noun: c is 4 words[c] is argo
__kubectl_handle_reply
__kubectl_parse_get: get completion by kubectl get --namespace=argo -o template --template="{{ range .items  }}{{ .metadata.name }} {{ end }}" "argo"
__kubectl_handle_word: c is 0 words[c] is kubectl
__kubectl_handle_command: c is 0 words[c] is kubectl
__kubectl_handle_command: looking for _kubectl_root_command
__kubectl_handle_word: c is 1 words[c] is get
__kubectl_handle_command: c is 1 words[c] is get
__kubectl_handle_command: looking for _kubectl_get
__kubectl_handle_word: c is 2 words[c] is pod
__kubectl_handle_noun: c is 2 words[c] is pod
__kubectl_handle_word: c is 3 words[c] is --namespace
__kubectl_handle_flag: c is 3 words[c] is --namespace
__kubectl_handle_flag: looking for --namespace
__kubectl_handle_word: c is 4 words[c] is argo
__kubectl_handle_noun: c is 4 words[c] is argo
__kubectl_handle_reply
__kubectl_parse_get: get completion by kubectl get --namespace=argo -o template --template="{{ range .items  }}{{ .metadata.name }} {{ end }}" "argo"
__kubectl_handle_word: c is 0 words[c] is kubectl
__kubectl_handle_command: c is 0 words[c] is kubectl
__kubectl_handle_command: looking for _kubectl_root_command
__kubectl_handle_word: c is 1 words[c] is get
__kubectl_handle_command: c is 1 words[c] is get
__kubectl_handle_command: looking for _kubectl_get
__kubectl_handle_word: c is 2 words[c] is -n
__kubectl_handle_flag: c is 2 words[c] is -n
__kubectl_handle_flag: looking for -n
__kubectl_handle_flag: found a flag -n, skip the next argument
__kubectl_handle_word: c is 4 words[c] is pod
__kubectl_handle_noun: c is 4 words[c] is pod
__kubectl_handle_reply
__kubectl_parse_get: get completion by kubectl get -n=argo -o template --template="{{ range .items  }}{{ .metadata.name }} {{ end }}" "pod"
__kubectl_handle_word: c is 0 words[c] is kubectl
__kubectl_handle_command: c is 0 words[c] is kubectl
__kubectl_handle_command: looking for _kubectl_root_command
__kubectl_handle_word: c is 1 words[c] is get
__kubectl_handle_command: c is 1 words[c] is get
__kubectl_handle_command: looking for _kubectl_get
__kubectl_handle_word: c is 2 words[c] is -n
__kubectl_handle_flag: c is 2 words[c] is -n
__kubectl_handle_flag: looking for -n
__kubectl_handle_flag: found a flag -n, skip the next argument
__kubectl_handle_word: c is 4 words[c] is pod
__kubectl_handle_noun: c is 4 words[c] is pod
__kubectl_handle_reply
__kubectl_parse_get: get completion by kubectl get -n=argo -o template --template="{{ range .items  }}{{ .metadata.name }} {{ end }}" "pod"
__kubectl_handle_word: c is 0 words[c] is kubectl
__kubectl_handle_command: c is 0 words[c] is kubectl
__kubectl_handle_command: looking for _kubectl_root_command
__kubectl_handle_word: c is 1 words[c] is get
__kubectl_handle_command: c is 1 words[c] is get
__kubectl_handle_command: looking for _kubectl_get
__kubectl_handle_word: c is 2 words[c] is --namespace=argo
__kubectl_handle_flag: c is 2 words[c] is --namespace=argo
__kubectl_handle_flag: looking for --namespace=
__kubectl_handle_word: c is 3 words[c] is pod
__kubectl_handle_noun: c is 3 words[c] is pod
__kubectl_handle_reply
__kubectl_parse_get: get completion by kubectl get --namespace=argo -o template --template="{{ range .items  }}{{ .metadata.name }} {{ end }}" "pod"
__kubectl_handle_word: c is 0 words[c] is kubectl
__kubectl_handle_command: c is 0 words[c] is kubectl
__kubectl_handle_command: looking for _kubectl_root_command
__kubectl_handle_word: c is 1 words[c] is get
__kubectl_handle_command: c is 1 words[c] is get
__kubectl_handle_command: looking for _kubectl_get
__kubectl_handle_word: c is 2 words[c] is --namespace=argo
__kubectl_handle_flag: c is 2 words[c] is --namespace=argo
__kubectl_handle_flag: looking for --namespace=
__kubectl_handle_word: c is 3 words[c] is pod
__kubectl_handle_noun: c is 3 words[c] is pod
__kubectl_handle_reply
__kubectl_parse_get: get completion by kubectl get --namespace=argo -o template --template="{{ range .items  }}{{ .metadata.name }} {{ end }}" "pod"
__kubectl_handle_word: c is 0 words[c] is kubectl
__kubectl_handle_command: c is 0 words[c] is kubectl
__kubectl_handle_command: looking for _kubectl_root_command
__kubectl_handle_word: c is 1 words[c] is get
__kubectl_handle_command: c is 1 words[c] is get
__kubectl_handle_command: looking for _kubectl_get
__kubectl_handle_word: c is 2 words[c] is --namespace
__kubectl_handle_flag: c is 2 words[c] is --namespace
__kubectl_handle_flag: looking for --namespace
__kubectl_handle_word: c is 3 words[c] is argo
__kubectl_handle_noun: c is 3 words[c] is argo
__kubectl_handle_word: c is 4 words[c] is pod
__kubectl_handle_noun: c is 4 words[c] is pod
__kubectl_handle_reply
__kubectl_parse_get: get completion by kubectl get --namespace=argo -o template --template="{{ range .items  }}{{ .metadata.name }} {{ end }}" "pod"
__kubectl_handle_word: c is 0 words[c] is kubectl
__kubectl_handle_command: c is 0 words[c] is kubectl
__kubectl_handle_command: looking for _kubectl_root_command
__kubectl_handle_word: c is 1 words[c] is get
__kubectl_handle_command: c is 1 words[c] is get
__kubectl_handle_command: looking for _kubectl_get
__kubectl_handle_word: c is 2 words[c] is --namespace
__kubectl_handle_flag: c is 2 words[c] is --namespace
__kubectl_handle_flag: looking for --namespace
__kubectl_handle_word: c is 3 words[c] is argo
__kubectl_handle_noun: c is 3 words[c] is argo
__kubectl_handle_word: c is 4 words[c] is pod
__kubectl_handle_noun: c is 4 words[c] is pod
__kubectl_handle_reply
__kubectl_parse_get: get completion by kubectl get --namespace=argo -o template --template="{{ range .items  }}{{ .metadata.name }} {{ end }}" "pod"

For the failed case of kubectl get pod --namespace argo, it executes get completion by kubectl get --namespace=argo -o template --template="{{ range .items }}{{ .metadata.name }} {{ end }}" "argo" to get the completions but the command has wrong resource name of argo which we expect pod.

I found the completion script has the following related lines.

...
    # skip the argument to a two word flag
    if __kubectl_contains_word "${words[c]}" "${two_word_flags[@]}"; then
        c=$((c+1))
        # if we are looking for a flags value, don't show commands
        if [[ $c -eq $cword ]]; then
            commands=()
        fi
    fi
...

This code skips next argument of two-word flags but it seems only -n is recognized as a two-word flag while --namespace is not.

...
    flags+=("--namespace=")
    flags_with_completion+=("--namespace")
    flags_completion+=("__kubectl_get_resource_namespace")
    two_word_flags+=("-n")
    flags_with_completion+=("-n")
    flags_completion+=("__kubectl_get_resource_namespace")
...

And I dug into the upstream code and found that part of the script is generated by cobra.

So I fixed it and sent a PR to cobra.
spf13/cobra#807

We can fix this issue by using a cobra version with this fix.

I actually have sent a PR to update the Cobra version which addresses another issue but doesn't include this fix.
#71447

I will fix either of the PRs depending on when new Cobra version will be released.

@dtaniwaki dtaniwaki changed the title Fix single dash flags handling Fix two-word flags handling Jan 12, 2019
@dtaniwaki dtaniwaki force-pushed the fix-auto-completion branch 2 times, most recently from ceab413 to e360424 Compare January 12, 2019 17:10
@dtaniwaki
Copy link
Contributor Author

/test pull-kubernetes-godeps

@k8s-ci-robot k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 11, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2019
@liggitt
Copy link
Member

liggitt commented May 16, 2019

golang.org/x/{sync,sys,tools,net} are currently on the go1.12 branch commits. no need to bump those.

@dtaniwaki
Copy link
Contributor Author

dtaniwaki commented May 16, 2019

@liggitt So do you mean I can ignore the result of ./hack/lint-dependencies.sh?

@liggitt
Copy link
Member

liggitt commented Jul 9, 2019

@liggitt So do you mean I can ignore the result of ./hack/lint-dependencies.sh?

for the golang.org/x/... dependencies, yes. Opened #79944 to omit those from the output by default to avoid confusing people.

It looks like cobra tagged a release containing the fix we want, which is great. You can replace all the dependency bump commits with a single commit that just bumps cobra:

hack/pin-dependency.sh github.com/spf13/cobra v0.0.5
hack/update-vendor.sh

@dtaniwaki
Copy link
Contributor Author

dtaniwaki commented Jul 10, 2019

Thank you for the information!

It looks the fix for two-word flags is included in cobra v0.0.4 and it has been updated in #78187 already although it doesn't mention the fix.

@dtaniwaki dtaniwaki changed the title Fix two-word flags handling Avoid conflicts with other cobra auto completions Jul 10, 2019
@k8s-ci-robot k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Jul 10, 2019
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 10, 2019
@dtaniwaki
Copy link
Contributor Author

@liggitt As the fix for two-word flags has been included in cobra v0.0.4 which has been updated in Kubernetes, I renamed the title of this PR and committed only the change of custom func name and debug mode autocompletion. Could you review it?

@liggitt
Copy link
Member

liggitt commented Jul 10, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dtaniwaki, liggitt

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 Jul 10, 2019
@k8s-ci-robot k8s-ci-robot merged commit 978c38d into kubernetes:master Jul 10, 2019
@dtaniwaki dtaniwaki deleted the fix-auto-completion branch July 28, 2019 00:52
@tatsuhiro-t
Copy link
Contributor

It looks like this PR breaks kubectl completion if override flags (e.g., --kubeconfig) are supplied.
With kubectl v1.16.0-alpha.2, kubectl --kubeconfig=foo -n then hit TAB will show "specify resource type" error.

@liggitt liggitt added this to the v1.16 milestone Aug 6, 2019
@liggitt liggitt removed the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Feb 18, 2020
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/apiserver area/cloudprovider area/code-generation area/dependency Issues or PRs related to dependency changes area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet