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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Mar 10, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion staging/src/k8s.io/kubectl/pkg/cmd/cmd.go
View file
Open in desktop
Expand Up @@ -500,7 +500,7 @@ func registerCompletionFuncForGlobalFlags(cmd *cobra.Command, f cmdutil.Factory)
cmdutil.CheckErr(cmd.RegisterFlagCompletionFunc(
"namespace",
func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
return utilcomp.CompGetResource(f, cmd, "namespace", toComplete), cobra.ShellCompDirectiveNoFileComp
return utilcomp.CompGetResource(f, "namespace", toComplete), cobra.ShellCompDirectiveNoFileComp
}))
cmdutil.CheckErr(cmd.RegisterFlagCompletionFunc(
"context",
Expand Down
6 changes: 3 additions & 3 deletions staging/src/k8s.io/kubectl/pkg/cmd/cp/cp.go
View file
Open in desktop
Expand Up @@ -109,14 +109,14 @@ func NewCmdCp(f cmdutil.Factory, ioStreams genericclioptions.IOStreams) *cobra.C
// complete <namespace>/<pod>
namespace := toComplete[:idx]
template := "{{ range .items }}{{ .metadata.namespace }}/{{ .metadata.name }}: {{ end }}"
comps = completion.CompGetFromTemplate(&template, f, namespace, cmd, []string{"pod"}, toComplete)
comps = completion.CompGetFromTemplate(&template, f, namespace, []string{"pod"}, toComplete)
} else {
// Complete namespaces followed by a /
for _, ns := range completion.CompGetResource(f, cmd, "namespace", toComplete) {
for _, ns := range completion.CompGetResource(f, "namespace", toComplete) {
comps = append(comps, fmt.Sprintf("%s/", ns))
}
// Complete pod names followed by a :
for _, pod := range completion.CompGetResource(f, cmd, "pod", toComplete) {
for _, pod := range completion.CompGetResource(f, "pod", toComplete) {
comps = append(comps, fmt.Sprintf("%s:", pod))
}

Expand Down
2 changes: 1 addition & 1 deletion staging/src/k8s.io/kubectl/pkg/cmd/create/create_clusterrolebinding.go
View file
Open in desktop
Expand Up @@ -108,7 +108,7 @@ func NewCmdCreateClusterRoleBinding(f cmdutil.Factory, ioStreams genericclioptio
cmdutil.CheckErr(cmd.RegisterFlagCompletionFunc(
"clusterrole",
func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
return completion.CompGetResource(f, cmd, "clusterrole", toComplete), cobra.ShellCompDirectiveNoFileComp
return completion.CompGetResource(f, "clusterrole", toComplete), cobra.ShellCompDirectiveNoFileComp
}))

return cmd
Expand Down
35 changes: 14 additions & 21 deletions staging/src/k8s.io/kubectl/pkg/cmd/get/get.go
View file
Open in desktop
Expand Up @@ -76,11 +76,11 @@ type GetOptions struct {
Namespace string
ExplicitNamespace bool
Subresource string
SortBy string

ServerPrint bool

NoHeaders bool
Sort bool
IgnoreNotFound bool

genericclioptions.IOStreams
Expand Down Expand Up @@ -170,7 +170,7 @@ func NewCmdGet(parent string, f cmdutil.Factory, streams genericclioptions.IOStr
Run: func(cmd *cobra.Command, args []string) {
cmdutil.CheckErr(o.Complete(f, cmd, args))
cmdutil.CheckErr(o.Validate())
cmdutil.CheckErr(o.Run(f, cmd, args))
cmdutil.CheckErr(o.Run(f, args))
},
SuggestFor: []string{"list", "ps"},
}
Expand Down Expand Up @@ -211,11 +211,9 @@ func (o *GetOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []stri
o.ExplicitNamespace = false
}

sortBy, err := cmd.Flags().GetString("sort-by")
if err != nil {
return err
if o.PrintFlags.HumanReadableFlags.SortBy != nil {
o.SortBy = *o.PrintFlags.HumanReadableFlags.SortBy
}
o.Sort = len(sortBy) > 0

o.NoHeaders = cmdutil.GetFlagBool(cmd, "no-headers")

Expand Down Expand Up @@ -264,8 +262,8 @@ func (o *GetOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []stri
return nil, err
}

if o.Sort {
printer = &SortingPrinter{Delegate: printer, SortField: sortBy}
if len(o.SortBy) > 0 {
printer = &SortingPrinter{Delegate: printer, SortField: o.SortBy}
}
if outputObjects != nil {
printer = &skipPrinter{delegate: printer, output: outputObjects}
Expand All @@ -278,7 +276,7 @@ func (o *GetOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []stri

switch {
case o.Watch || o.WatchOnly:
if o.Sort {
if len(o.SortBy) > 0 {
fmt.Fprintf(o.IOStreams.ErrOut, "warning: --watch or --watch-only requested, --sort-by will be ignored\n")
}
default:
Expand Down Expand Up @@ -448,14 +446,14 @@ func (o *GetOptions) transformRequests(req *rest.Request) {
}, ","))

// if sorting, ensure we receive the full object in order to introspect its fields via jsonpath
if o.Sort {
if len(o.SortBy) > 0 {
req.Param("includeObject", "Object")
}
}

// Run performs the get operation.
// TODO: remove the need to pass these arguments, like other commands.
func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) error {
func (o *GetOptions) Run(f cmdutil.Factory, args []string) error {
if len(o.Raw) > 0 {
restClient, err := f.RESTClient()
if err != nil {
Expand All @@ -464,11 +462,11 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e
return rawhttp.RawGet(restClient, o.IOStreams, o.Raw)
}
if o.Watch || o.WatchOnly {
return o.watch(f, cmd, args)
return o.watch(f, args)
}

chunkSize := o.ChunkSize
if o.Sort {
if len(o.SortBy) > 0 {
// TODO(juanvallejo): in the future, we could have the client use chunking
// to gather all results, then sort them all at the end to reduce server load.
chunkSize = 0
Expand Down Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if err != nil {
return err
}

var positioner OriginalPositioner
if o.Sort {
sorter := NewRuntimeSorter(objs, sorting)
if len(o.SortBy) > 0 {
sorter := NewRuntimeSorter(objs, o.SortBy)
if err := sorter.Sort(); err != nil {
return err
}
Expand Down Expand Up @@ -632,7 +625,7 @@ func (s *separatorWriterWrapper) SetReady(state bool) {

// watch starts a client-side watch of one or more resources.
// TODO: remove the need for arguments here.
func (o *GetOptions) watch(f cmdutil.Factory, cmd *cobra.Command, args []string) error {
func (o *GetOptions) watch(f cmdutil.Factory, args []string) error {
r := f.NewBuilder().
Unstructured().
NamespaceParam(o.Namespace).DefaultNamespace().AllNamespaces(o.AllNamespaces).
Expand Down
47 changes: 24 additions & 23 deletions staging/src/k8s.io/kubectl/pkg/util/completion/completion.go
View file
Open in desktop
Expand Up @@ -25,6 +25,7 @@ import (
"time"

"github.com/spf13/cobra"

"k8s.io/apimachinery/pkg/api/meta"
"k8s.io/cli-runtime/pkg/genericclioptions"
"k8s.io/cli-runtime/pkg/printers"
Expand Down Expand Up @@ -69,10 +70,10 @@ func SpecifiedResourceTypeAndNameNoRepeatCompletionFunc(f cmdutil.Factory, allow
// that don't support that form. For commands that apply to pods and that support the <type>/<name>
// form, please use PodResourceNameCompletionFunc()
func ResourceNameCompletionFunc(f cmdutil.Factory, resourceType string) 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) {
var comps []string
if len(args) == 0 {
comps = CompGetResource(f, cmd, resourceType, toComplete)
comps = CompGetResource(f, resourceType, toComplete)
}
return comps, cobra.ShellCompDirectiveNoFileComp
}
Expand All @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I see it now, thanks.

var comps []string
directive := cobra.ShellCompDirectiveNoFileComp
if len(args) == 0 {
comps, directive = doPodResourceCompletion(f, cmd, toComplete)
comps, directive = doPodResourceCompletion(f, toComplete)
}
return comps, directive
}
Expand All @@ -97,14 +98,14 @@ func PodResourceNameCompletionFunc(f cmdutil.Factory) func(*cobra.Command, []str
// 2- resource types containing pods which match the toComplete prefix
// and as a second argument the containers within the specified pod.
func PodResourceNameAndContainerCompletionFunc(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) {
var comps []string
directive := cobra.ShellCompDirectiveNoFileComp
if len(args) == 0 {
comps, directive = doPodResourceCompletion(f, cmd, toComplete)
comps, directive = doPodResourceCompletion(f, toComplete)
} else if len(args) == 1 {
podName := convertResourceNameToPodName(f, args[0])
comps = CompGetContainers(f, cmd, podName, toComplete)
comps = CompGetContainers(f, podName, toComplete)
}
return comps, directive
}
Expand All @@ -114,21 +115,21 @@ func PodResourceNameAndContainerCompletionFunc(f cmdutil.Factory) func(*cobra.Co
// pod specified by the first argument. The resource containing the pod can be specified in
// the <type>/<name> form.
func ContainerCompletionFunc(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) {
var comps []string
// We need the pod name to be able to complete the container names, it must be in args[0].
// That first argument can also be of the form <type>/<name> so we need to convert it.
if len(args) > 0 {
podName := convertResourceNameToPodName(f, args[0])
comps = CompGetContainers(f, cmd, podName, toComplete)
comps = CompGetContainers(f, podName, toComplete)
}
return comps, cobra.ShellCompDirectiveNoFileComp
}
}

// ContextCompletionFunc is a completion function that completes as a first argument the
// context names that match the toComplete prefix
func ContextCompletionFunc(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
func ContextCompletionFunc(_ *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
if len(args) == 0 {
return ListContextsInConfig(toComplete), cobra.ShellCompDirectiveNoFileComp
}
Expand All @@ -137,7 +138,7 @@ func ContextCompletionFunc(cmd *cobra.Command, args []string, toComplete string)

// ClusterCompletionFunc is a completion function that completes as a first argument the
// cluster names that match the toComplete prefix
func ClusterCompletionFunc(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
func ClusterCompletionFunc(_ *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
if len(args) == 0 {
return ListClustersInConfig(toComplete), cobra.ShellCompDirectiveNoFileComp
}
Expand All @@ -146,28 +147,28 @@ func ClusterCompletionFunc(cmd *cobra.Command, args []string, toComplete string)

// UserCompletionFunc is a completion function that completes as a first argument the
// user names that match the toComplete prefix
func UserCompletionFunc(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
func UserCompletionFunc(_ *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
if len(args) == 0 {
return ListUsersInConfig(toComplete), cobra.ShellCompDirectiveNoFileComp
}
return nil, cobra.ShellCompDirectiveNoFileComp
}

// CompGetResource gets the list of the resource specified which begin with `toComplete`.
func CompGetResource(f cmdutil.Factory, cmd *cobra.Command, resourceName string, toComplete string) []string {
func CompGetResource(f cmdutil.Factory, resourceName string, toComplete string) []string {
template := "{{ range .items }}{{ .metadata.name }} {{ end }}"
return CompGetFromTemplate(&template, f, "", cmd, []string{resourceName}, toComplete)
return CompGetFromTemplate(&template, f, "", []string{resourceName}, toComplete)
}

// CompGetContainers gets the list of containers of the specified pod which begin with `toComplete`.
func CompGetContainers(f cmdutil.Factory, cmd *cobra.Command, podName string, toComplete string) []string {
func CompGetContainers(f cmdutil.Factory, podName string, toComplete string) []string {
template := "{{ range .spec.initContainers }}{{ .name }} {{end}}{{ range .spec.containers }}{{ .name }} {{ end }}"
return CompGetFromTemplate(&template, f, "", cmd, []string{"pod", podName}, toComplete)
return CompGetFromTemplate(&template, f, "", []string{"pod", podName}, toComplete)
}

// CompGetFromTemplate executes a Get operation using the specified template and args and returns the results
// which begin with `toComplete`.
func CompGetFromTemplate(template *string, f cmdutil.Factory, namespace string, cmd *cobra.Command, args []string, toComplete string) []string {
func CompGetFromTemplate(template *string, f cmdutil.Factory, namespace string, args []string, toComplete string) []string {
buf := new(bytes.Buffer)
streams := genericclioptions.IOStreams{In: os.Stdin, Out: buf, ErrOut: io.Discard}
o := get.NewGetOptions("kubectl", streams)
Expand Down Expand Up @@ -199,7 +200,7 @@ func CompGetFromTemplate(template *string, f cmdutil.Factory, namespace string,
return printer.PrintObj, nil
}

o.Run(f, cmd, args)
o.Run(f, args)

var comps []string
resources := strings.Split(buf.String(), " ")
Expand Down Expand Up @@ -304,7 +305,7 @@ func resourceTypeAndNameCompletionFunc(f cmdutil.Factory, allowedTypes []string,
// The first argument is of the form <type> (e.g., pods)
// All following arguments should be a resource name.
if allowRepeat || len(args) == 1 {
comps = CompGetResource(f, cmd, args[0], toComplete)
comps = CompGetResource(f, args[0], toComplete)

// Remove choices already on the command-line
if len(args) > 1 {
Expand Down Expand Up @@ -356,7 +357,7 @@ func resourceTypeAndNameCompletionFunc(f cmdutil.Factory, allowedTypes []string,
if allowRepeat || len(args) == 0 {
resourceType := toComplete[:slashIdx]
toComplete = toComplete[slashIdx+1:]
nameComps := CompGetResource(f, cmd, resourceType, toComplete)
nameComps := CompGetResource(f, resourceType, toComplete)
for _, c := range nameComps {
comps = append(comps, fmt.Sprintf("%s/%s", resourceType, c))
}
Expand All @@ -375,13 +376,13 @@ func resourceTypeAndNameCompletionFunc(f cmdutil.Factory, allowedTypes []string,
// doPodResourceCompletion Returns completions of:
// 1- pod names that match the toComplete prefix
// 2- resource types containing pods which match the toComplete prefix
func doPodResourceCompletion(f cmdutil.Factory, cmd *cobra.Command, toComplete string) ([]string, cobra.ShellCompDirective) {
func doPodResourceCompletion(f cmdutil.Factory, toComplete string) ([]string, cobra.ShellCompDirective) {
var comps []string
directive := cobra.ShellCompDirectiveNoFileComp
slashIdx := strings.Index(toComplete, "/")
if slashIdx == -1 {
// Standard case, complete pod names
comps = CompGetResource(f, cmd, "pod", toComplete)
comps = CompGetResource(f, "pod", toComplete)

// Also include resource choices for the <type>/<name> form,
// but only for resources that contain pods
Expand Down Expand Up @@ -410,7 +411,7 @@ func doPodResourceCompletion(f cmdutil.Factory, cmd *cobra.Command, toComplete s
// Dealing with the <type>/<name> form, use the specified resource type
resourceType := toComplete[:slashIdx]
toComplete = toComplete[slashIdx+1:]
nameComps := CompGetResource(f, cmd, resourceType, toComplete)
nameComps := CompGetResource(f, resourceType, toComplete)
for _, c := range nameComps {
comps = append(comps, fmt.Sprintf("%s/%s", resourceType, c))
}
Expand Down