Shell completion of multiple resource names #105711

marckhouzam · 2021-10-16T02:17:22Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR teaches the completion functions to repeat resource names when applicable and supported. The logic checks if a resource name has already been specified by the user and does not include it again when repeating the completion.

For example, the get command can receive multiple pods names, therefore with this commit:
kubectl get pod pod1 [tab]
will provide completion of pod names again, but not show pod1 a second time since it is already part of the command-line.

The improvement affects the following commands:

annotate
apply edit-last-applied
apply view-last-applied
autoscale
delete
describe
edit
expose
get
label
patch
rollout history
rollout pause
rollout restart
rollout resume
rollout undo
scale
taint

Note that "rollout status" only accepts a single resource name, unlike the other "rollout ..." commands; this required the creation of a special completion function that did not repeat just for that case.

Which issue(s) this PR fixes:

Replaces #101427

Special notes for your reviewer:

The PR adds a utility function to remove a list of values from an array. It is used to prevent the completion from suggesting choices that are already on the command-line (as explained above). I don't know if such a utility already existed in the code base or if I put it in the right place (cmd/util/helpers.go#Unique(..))

Does this PR introduce a user-facing change?

Shell completion now knows to continue suggesting resource names when the command supports it.  For example "kubectl get pod pod1 <TAB>" will suggest more pod names.

/cc @phil9909

k8s-ci-robot · 2021-10-16T02:17:24Z

@marckhouzam: GitHub didn't allow me to request PR reviews from the following users: phil9909.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR teaches the completion functions to repeat resource names when applicable and supported. The logic checks if a resource name has already been specified by the user and does not include it again when repeating the completion.

For example, the get command can receive multiple pods names, therefore with this commit:
kubectl get pod pod1 [tab]
will provide completion of pod names again, but not show pod1 a second time since it is already part of the command-line.

The improvement affects the following commands:

annotate

apply edit-last-applied

apply view-last-applied

autoscale

delete

describe

edit

expose

get

label

patch

rollout history

rollout pause

rollout restart

rollout resume

rollout undo

scale

taint

Note that "rollout status" only accepts a single resource name, unlike the other "rollout ..." commands; this required the creation of a special completion function that did not repeat just for that case.

Which issue(s) this PR fixes:

Replaces #101427

Special notes for your reviewer:

The PR adds a utility function to remove a list of values from an array. It is used to prevent the completion from suggesting choices that are already on the command-line (as explained above). I don't know if such a utility already existed in the code base or if I put it in the right place (cmd/util/helpers.go#Unique(..))

Does this PR introduce a user-facing change?
Shell completion now knows to continue suggesting resource names when the command supports it.  For example "kubectl get pod pod1 <TAB>" will suggest more pod names.
/cc @phil9909

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 · 2021-10-16T02:17:30Z

@marckhouzam: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 · 2021-10-16T02:17:30Z

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.

marckhouzam · 2021-10-16T02:19:33Z

/sig cli

lauchokyip · 2021-10-20T16:12:23Z

/ok-to-test
/cc @brianpursley

brianpursley · 2021-10-21T03:37:00Z

/retest

brianpursley

Looks good. I left a few comments, but this looks like a good improvement to completions.

brianpursley · 2021-10-23T13:20:54Z

staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go

@@ -733,3 +733,21 @@ func Warning(cmdErr io.Writer, newGeneratorName, oldGeneratorName string) {
 		oldGeneratorName,
 	)
 }
+
+// Unique removes any elements of subArray from fullArray and returns the result
+func Unique(fullArray []string, subArray []string) []string {


It took me a couple of looks to realize what this function is doing.

It is called Unique but it does not de-duplicate items in the fullArray, it only removes them if their exist in subArray. That's fine, but I think the name could be more accurate for its purpose.

Might I suggest Exclude() as a name? Or Difference() (which I think would be the term for this in set theory)?

Good point. Unique() is a very poor name in this case. Let's go with Difference() since it is used in Set theory.

brianpursley · 2021-10-23T13:35:34Z

staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go

+	var result []string
+	for _, elem := range fullArray {
+		found := false
+		for _, subElem := range subArray {


I know subArray is usually going to be pretty small in your particular use case, but I think this could be a little simpler and more efficient by using a map instead of nested loops.

For example, something like this is what I'm thinking:

exclude := make(map[string]bool) for _, elem := range subArray { exclude[elem] = true } var result []string for _, elem := range fullArray { if _, found := exclude[elem]; !found { result = append(result, elem) } } return result

Good optimization. I will use it. Thanks.

brianpursley · 2021-10-23T13:36:58Z

staging/src/k8s.io/kubectl/pkg/cmd/util/helpers.go

@@ -733,3 +733,21 @@ func Warning(cmdErr io.Writer, newGeneratorName, oldGeneratorName string) {
 		oldGeneratorName,
 	)
 }
+
+// Unique removes any elements of subArray from fullArray and returns the result
+func Unique(fullArray []string, subArray []string) []string {


Can we get a unit test for this function?

Done.
I needed a function to check if two arrays were equal so I created checkArrayEqual(), but if you know of an existing function that does that, please let me know.

marckhouzam · 2021-10-23T23:57:17Z

Thanks for the review @brianpursley, I believe the latest version addresses your comments.

brianpursley

Looks good. Tried it out locally and seems to work well. Nice work.

marckhouzam · 2021-10-26T10:26:07Z

/assign @eddiezane

eddiezane

This is awesome! Thanks for adding. A few nits to address and then lgtm.

eddiezane · 2021-10-26T22:09:36Z

staging/src/k8s.io/kubectl/pkg/cmd/util/helpers_test.go

@@ -321,3 +322,43 @@ func TestDumpReaderToFile(t *testing.T) {
 		t.Fatalf("Wrong file content %s != %s", testString, stringData)
 	}
 }
+
+func TestDifferenceFunc(t *testing.T) {


nit: Let's use table driven tests here.

Much better. Thanks for pointing it out.

eddiezane · 2021-10-26T22:10:17Z

staging/src/k8s.io/kubectl/pkg/cmd/util/helpers_test.go

+	}
+}
+
+func checkArrayEqual(arr1, arr2 []string) bool {


We already use go-cmp in kubectl and can use it here.

eddiezane · 2021-10-26T22:30:19Z

staging/src/k8s.io/kubectl/pkg/util/completion_test.go

 	defer tf.Cleanup()

+	pods, _, _ := cmdtesting.TestData()


Can we use

tf, cmd := prepareCompletionTest() addPodsToFactory(tf)

like above for these three tests?

I don't know what happened there. I created the common functions when moving to Go completions but forgot to use them for the other tests 😮

I fixed all of them. Thanks for catching that!

This commit teaches the completion function to repeat resource names when supported by the command. The logic checks if a resource name has already been specified by the user and does not include it again when repeating the completion. For example, the get command can receive multiple pods names, therefore with this commit we have: kubectl get pod pod1 [tab] will provide completion of pod names again, but not show 'pod1' since it is already part of the command-line. The improvement affects the following commands: - annotate - apply edit-last-applied - apply view-last-applied - autoscale - delete - describe - edit - expose - get - label - patch - rollout history - rollout pause - rollout restart - rollout resume - rollout undo - scale - taint Note that "rollout status" only accepts a single resource name, unlike the other "rollout ..." commands; this required the creation of a special completion function that did not repeat just for that case. Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>

marckhouzam · 2021-10-27T01:52:25Z

Ready again @eddiezane

eddiezane

/lgtm
/approve

k8s-ci-robot · 2021-10-27T14:53:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley, eddiezane, marckhouzam

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:

~~staging/src/k8s.io/kubectl/OWNERS~~ [eddiezane]

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

marckhouzam · 2021-10-27T16:55:54Z

/retest

eddiezane · 2021-10-27T17:35:17Z

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/105711/pull-kubernetes-integration/1453389379044118528 flake is #103512

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 16, 2021

k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. area/kubectl and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 16, 2021

k8s-ci-robot requested a review from brianpursley October 20, 2021 16:12

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 20, 2021

brianpursley suggested changes Oct 23, 2021

View reviewed changes

marckhouzam force-pushed the feat/multiComp branch from e3d6d2d to d982df1 Compare October 23, 2021 23:53

brianpursley approved these changes Oct 26, 2021

View reviewed changes

k8s-ci-robot assigned eddiezane Oct 26, 2021

eddiezane reviewed Oct 26, 2021

View reviewed changes

marckhouzam force-pushed the feat/multiComp branch from d982df1 to 7aa5cb4 Compare October 27, 2021 01:51

eddiezane approved these changes Oct 27, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 27, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2021

k8s-ci-robot merged commit aa7c633 into kubernetes:master Oct 27, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 27, 2021

marckhouzam deleted the feat/multiComp branch October 27, 2021 17:55

marckhouzam mentioned this pull request Oct 30, 2021

Fix bash completion for multiple resource names #101427

Closed

marckhouzam mentioned this pull request Mar 3, 2022

REQUEST: New membership for marckhouzam kubernetes/org#3301

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shell completion of multiple resource names #105711

Shell completion of multiple resource names #105711

marckhouzam commented Oct 16, 2021

k8s-ci-robot commented Oct 16, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Oct 16, 2021

k8s-ci-robot commented Oct 16, 2021

marckhouzam commented Oct 16, 2021

lauchokyip commented Oct 20, 2021

brianpursley commented Oct 21, 2021

brianpursley left a comment

brianpursley Oct 23, 2021

marckhouzam Oct 23, 2021

brianpursley Oct 23, 2021

marckhouzam Oct 23, 2021

brianpursley Oct 23, 2021

marckhouzam Oct 23, 2021

marckhouzam commented Oct 23, 2021

brianpursley left a comment

marckhouzam commented Oct 26, 2021

eddiezane left a comment

eddiezane Oct 26, 2021

marckhouzam Oct 27, 2021

eddiezane Oct 26, 2021

marckhouzam Oct 27, 2021

eddiezane Oct 26, 2021

marckhouzam Oct 27, 2021

marckhouzam commented Oct 27, 2021

eddiezane left a comment

k8s-ci-robot commented Oct 27, 2021

marckhouzam commented Oct 27, 2021

eddiezane commented Oct 27, 2021

Shell completion of multiple resource names #105711

Shell completion of multiple resource names #105711

Conversation

marckhouzam commented Oct 16, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Oct 16, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Oct 16, 2021

k8s-ci-robot commented Oct 16, 2021

marckhouzam commented Oct 16, 2021

lauchokyip commented Oct 20, 2021

brianpursley commented Oct 21, 2021

brianpursley 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marckhouzam commented Oct 23, 2021

brianpursley left a comment

Choose a reason for hiding this comment

marckhouzam commented Oct 26, 2021

eddiezane 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marckhouzam commented Oct 27, 2021

eddiezane left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 27, 2021

marckhouzam commented Oct 27, 2021

eddiezane commented Oct 27, 2021