Implement custom completions for some flags using Go

[knight42]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implement custom completions using Go for flag --context, --cluster, --user and --namespace.

Which issue(s) this PR fixes:

Part of kubernetes/kubectl#882

Special notes for your reviewer:

Test the new completion code

1. build kubectl from source

make WHAT=cmd/kubectl

2. generate completion code

# zsh
source <(./_output/bin/kubectl completion zsh)

# bash
source <(./_output/bin/kubectl completion bash)

3. play with it

Please note that the new completion code requires the new kubectl.

./_output/bin/kubectl get --context [TAB]

# debug
./_output/bin/kubectl __complete get --context ''

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[knight42]

/cc @brianpursley @marckhouzam

PTAL. You might be only interested in this commit: 58e57ee

[marckhouzam]

@knight42 how are things going for these Go completions?

[knight42]

@marckhouzam Oh I think I have missed the comments from @brianpursley and just waiting for review. I am still working on it.

[knight42]

For reviewer: I move helper functions here as I think they might be reused for the completion of kubectl config get-{contexts,clusters}

[k8s-ci-robot]

@knight42: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-csi-serial	04c8a8c46dd9647532fa65321755c2d4b9807d84	link	/test pull-kubernetes-e2e-gce-csi-serial
pull-kubernetes-e2e-gci-gce-ipvs	04c8a8c46dd9647532fa65321755c2d4b9807d84	link	/test pull-kubernetes-e2e-gci-gce-ipvs
pull-kubernetes-e2e-gce-storage-slow	04c8a8c46dd9647532fa65321755c2d4b9807d84	link	/test pull-kubernetes-e2e-gce-storage-slow

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: knight42
To complete the pull request process, please assign soltysh
You can assign the PR to them by writing /assign @soltysh in a comment when ready.

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

Needs approval from an approver in each of these files:

• pkg/kubectl/OWNERS
• staging/src/k8s.io/kubectl/OWNERS
• vendor/OWNERS

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

[brianpursley]

@knight42 any update on this?

I think this is a good start, but what else would need to be implemented in order to get rid of the custom bash and zsh code in these two locations?

• https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cmd.go#L83-L289
• https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/kubectl/pkg/cmd/completion/completion.go#L146-L268

I think below is the key function. It looks like there are 7 completion cases that need to be implemented and then we could get rid of the above two blocks of code I linked to...

__kubectl_custom_func() {
    case ${last_command} in
        kubectl_get | kubectl_describe | kubectl_delete | kubectl_label | kubectl_edit | kubectl_patch |\
        kubectl_annotate | kubectl_expose | kubectl_scale | kubectl_autoscale | kubectl_taint | kubectl_rollout_* |\
        kubectl_apply_edit-last-applied | kubectl_apply_view-last-applied)
            __kubectl_get_resource
            return
            ;;
        kubectl_logs)
            __kubectl_require_pod_and_container
            return
            ;;
        kubectl_exec | kubectl_port-forward | kubectl_top_pod | kubectl_attach)
            __kubectl_get_resource_pod
            return
            ;;
        kubectl_cordon | kubectl_uncordon | kubectl_drain | kubectl_top_node)
            __kubectl_get_resource_node
            return
            ;;
        kubectl_config_use-context | kubectl_config_rename-context)
            __kubectl_config_get_contexts
            return
            ;;
        kubectl_config_delete-cluster)
            __kubectl_config_get_clusters
            return
            ;;
        kubectl_cp)
            __kubectl_cp
            return
            ;;
        *)
            ;;
    esac
}

Types of completions:

• resources
• pods & containers
• pods
• nodes
• contexts <-- you already have the function for this one done
• clusters <-- you already have the function for this one done
• cp

[knight42]

@brianpursley The original target of this PR is to only implement custom completions using Go for flag --context, --cluster, --user and --namespace. I think it is completed now.

but what else would need to be implemented in order to get rid of the custom bash and zsh code in these two locations?

We need to modify each command as described in https://github.com/spf13/cobra/blob/master/shell_completions.md#dynamic-completion-of-nouns. I plan to open separate PR for each command, perhaps we could open a tracking issue and call for community participation.

[marckhouzam]

I feel this would have value to merge as is.

It will work for both bash and zsh and is a step in the right direction to get rid of the custom bash code.

And it will work automatically for fish completion if/when it is added (#92989)

[brianpursley]

I'm not trying to be difficult, but I am hesitant to merge this because it does not fully address the issue. I don't really like the idea of having some of the completions in go, but others still using custom scripts. I was hoping to see a PR that fully addresses the issue.

However, if someone with the ability to approve PRs is comfortable with doing it though, then I will go along with their opinion on this, as I don't want to make this a sticking point if everyone else sees things differently.

Either way, we need to make 100% sure this doesn't break existing completions for zsh and bash. This probably needs some manual confirmation as I don't think we have a way to test completions using integration tests. Also, when go completions are created, we should be able to have go unit tests for those now, so that might help too.

My number one concern is not to break existing completions which I know tool a long time and a lot of work to get working satisfactorily.

[marckhouzam]

@brianpursley I agree with your concerns about making sure this doesn't break anything. That is why I personally like the gradual approach so as to make sure each change is safe.

That being said, you are also right that temporarily having half the completions in go and the other half in bash will still cause some confusion.

@knight42 are you hoping to continue this work for the rest of the custom completions? I would like to help but I worry that my total lack of knowledge of the kubectl code base will prevent me from suggesting the correct go code for the completions.

[marckhouzam]

@brianpursley I've built upon @knight42 work and removed all bash scripting in #96087.

[fejta-bot]

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]

@knight42: 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.

[knight42]

/close
in favor of #96087