cmd/get: Remove cmd argument from Run() #114682

brianpursley · 2022-12-23T19:40:34Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Removes the need to pass cmd as an argument to Run(). This change required reading the --sort-by flag in Complete() in a way similar to other flags.

This change allows the cobra.Command not to need to be passed throughout the completion code, which I updated as part of this commit.

It also is a step in the direction of the TODO comment requesting the removal of arguments passed to Run() and watch().

Which issue(s) this PR fixes:

Indirectly related to kubernetes/kubectl#1046

Special notes for your reviewer:

The motivation for this is, I want to get to a place where factory can be removed from the completion package. This is a step towards that.

Does this PR introduce a user-facing change?

NONE

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

brianpursley · 2022-12-23T19:40:56Z

/hold
for review

Removes the need to pass cmd as an argument to Run(). This change required reading the --sort-by flag in Complete() in a way similar to other flags. This change allows the cobra.Command not to need to be passed throughout the completion code, which I updated as part of this commit. It also is a step in the direction of the TODO comment requesting the removal of arguments passed to Run() and watch().

brianpursley · 2022-12-23T21:31:15Z

/retest

mpuckett159 · 2022-12-29T23:15:55Z

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

@@ -82,11 +83,11 @@ func ResourceNameCompletionFunc(f cmdutil.Factory, resourceType string) func(*co
 // 1- pod names that match the toComplete prefix
 // 2- resource types containing pods which match the toComplete prefix
 func PodResourceNameCompletionFunc(f cmdutil.Factory) func(*cobra.Command, []string, string) ([]string, cobra.ShellCompDirective) {
-	return func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
+	return func(_ *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {


Sorry if this is dumb but what's the point of this? Is it just because this is possibly being used elsewhere and you just want to throw the first function input away, because it would drastically balloon the PR if you went around and updated the usages everywhere in the code base?

The underscore you mean? It just explicitly indicates that the parameter is unused. If I recall correctly, the function signature is defined by cobra, so it can't be changed, but we are not using the cmd arg.

Oh I see it now, thanks.

ardaguclu · 2023-01-02T07:16:28Z

staging/src/k8s.io/kubectl/pkg/cmd/get/get.go

@@ -513,14 +511,9 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e
 		objs[ix] = infos[ix].Object
 	}

-	sorting, err := cmd.Flags().GetString("sort-by")


Now we are removing this line because it is already set in Complete. But we are not executing Complete function in here

kubernetes/staging/src/k8s.io/kubectl/pkg/util/completion/completion.go

Line 180 in 774ab13

// Do the steps Complete() would have done.

Maybe completions do not use sort-by flag and this change will not create any problem?

I don't think allowing users to customize sorting for completions is really necessary. This seems fine to me. If we wanted to add it later we could I guess. Just set the o.SortBy manually in the completion code.

mpuckett159 · 2023-01-03T21:59:49Z

/lgtm imo. Reminding me that I really need to work on mine for config.

mpuckett159 · 2023-01-03T22:01:32Z

/lgtm
Does it need to be on it's own line?

k8s-ci-robot · 2023-01-03T22:01:39Z

LGTM label has been added.

Git tree hash: 0cd72da43bd1d52951cd26e13a8e5f491113761d

dims · 2023-02-20T16:23:31Z

/assign @soltysh

soltysh

/lgtm
/hold cancel

soltysh · 2023-03-09T16:06:52Z

/triage accepted
/priority backlog

k8s-ci-robot · 2023-03-09T16:06:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brianpursley, 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:

~~staging/src/k8s.io/kubectl/OWNERS~~ [brianpursley,soltysh]

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

k8s-triage-robot · 2023-03-09T19:15:39Z

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

The PR does have any do-not-merge/* labels
The PR does not have the needs-ok-to-test label
The PR is mergeable (does not have a needs-rebase label)
The PR is approved (has cncf-cla: yes, lgtm, approved labels)
The PR is failing tests required for merge

You can:

Review the full test history for this PR
Prevent this bot from retesting with /lgtm cancel or /hold
Help make our tests less flaky by following our Flaky Tests Guide

/retest

k8s-ci-robot requested review from mpuckett159 and seans3 December 23, 2022 19:42

brianpursley changed the title ~~cmd/get: Remove cmd argument from Run()~~ WIP: cmd/get: Remove cmd argument from Run() Dec 23, 2022

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 23, 2022

brianpursley marked this pull request as draft December 23, 2022 19:49

brianpursley force-pushed the get-cmd-arg branch from 8c8fdcb to aa7a828 Compare December 23, 2022 19:57

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 23, 2022

brianpursley marked this pull request as ready for review December 23, 2022 20:38

brianpursley changed the title ~~WIP: cmd/get: Remove cmd argument from Run()~~ cmd/get: Remove cmd argument from Run() Dec 23, 2022

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 23, 2022

k8s-ci-robot requested review from KnVerey and soltysh December 23, 2022 20:38

mpuckett159 reviewed Dec 29, 2022

View reviewed changes

ardaguclu reviewed Jan 2, 2023

View reviewed changes

k8s-ci-robot assigned mpuckett159 Jan 3, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 3, 2023

k8s-ci-robot assigned soltysh Feb 20, 2023

soltysh approved these changes Mar 9, 2023

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2023

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. labels Mar 9, 2023

k8s-ci-robot removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 9, 2023

k8s-ci-robot merged commit 315f94d into kubernetes:master Mar 10, 2023

k8s-ci-robot added this to the v1.27 milestone Mar 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/get: Remove cmd argument from Run() #114682

cmd/get: Remove cmd argument from Run() #114682

brianpursley commented Dec 23, 2022 •

edited

Loading

brianpursley commented Dec 23, 2022

brianpursley commented Dec 23, 2022

mpuckett159 Dec 29, 2022 •

edited

Loading

brianpursley Dec 29, 2022

mpuckett159 Dec 31, 2022

ardaguclu Jan 2, 2023

mpuckett159 Jan 3, 2023 •

edited

Loading

mpuckett159 commented Jan 3, 2023

mpuckett159 commented Jan 3, 2023

k8s-ci-robot commented Jan 3, 2023

dims commented Feb 20, 2023

soltysh left a comment

soltysh commented Mar 9, 2023

k8s-ci-robot commented Mar 9, 2023

k8s-triage-robot commented Mar 9, 2023

cmd/get: Remove cmd argument from Run() #114682

cmd/get: Remove cmd argument from Run() #114682

Conversation

brianpursley commented Dec 23, 2022 • edited Loading

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?

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

brianpursley commented Dec 23, 2022

brianpursley commented Dec 23, 2022

mpuckett159 Dec 29, 2022 • edited Loading

Choose a reason for hiding this comment

brianpursley Dec 29, 2022

Choose a reason for hiding this comment

mpuckett159 Dec 31, 2022

Choose a reason for hiding this comment

ardaguclu Jan 2, 2023

Choose a reason for hiding this comment

mpuckett159 Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

mpuckett159 commented Jan 3, 2023

mpuckett159 commented Jan 3, 2023

k8s-ci-robot commented Jan 3, 2023

dims commented Feb 20, 2023

soltysh left a comment

Choose a reason for hiding this comment

soltysh commented Mar 9, 2023

k8s-ci-robot commented Mar 9, 2023

k8s-triage-robot commented Mar 9, 2023

brianpursley commented Dec 23, 2022 •

edited

Loading

mpuckett159 Dec 29, 2022 •

edited

Loading

mpuckett159 Jan 3, 2023 •

edited

Loading