From c5c88006efed2aca44cacd4ad93e88b9150d6b7b Mon Sep 17 00:00:00 2001 From: yue9944882 <291271447@qq.com> Date: Tue, 26 Jun 2018 22:06:20 +0800 Subject: [PATCH] show kind for multiple resource types review: simplify and trim codes refactor ToPrinter and pass show kind flag remove tests together with removed function --- pkg/kubectl/cmd/get/BUILD | 5 ++ pkg/kubectl/cmd/get/get.go | 52 ++++++------- pkg/kubectl/cmd/get/get_test.go | 64 +++++++++++++++ pkg/kubectl/cmd/testing/fake.go | 18 +++++ .../genericclioptions/resource/builder.go | 36 +-------- .../resource/builder_test.go | 77 ------------------- 6 files changed, 114 insertions(+), 138 deletions(-) diff --git a/pkg/kubectl/cmd/get/BUILD b/pkg/kubectl/cmd/get/BUILD index a0fa777afab..308d458c9a0 100644 --- a/pkg/kubectl/cmd/get/BUILD +++ b/pkg/kubectl/cmd/get/BUILD @@ -77,7 +77,12 @@ go_test( "//pkg/kubectl/genericclioptions:go_default_library", "//pkg/kubectl/genericclioptions/resource:go_default_library", "//pkg/kubectl/scheme:go_default_library", + "//staging/src/k8s.io/api/apps/v1:go_default_library", + "//staging/src/k8s.io/api/autoscaling/v1:go_default_library", + "//staging/src/k8s.io/api/batch/v1:go_default_library", + "//staging/src/k8s.io/api/batch/v1beta1:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", + "//staging/src/k8s.io/api/extensions/v1beta1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", diff --git a/pkg/kubectl/cmd/get/get.go b/pkg/kubectl/cmd/get/get.go index c629b110776..0a3d2714b85 100644 --- a/pkg/kubectl/cmd/get/get.go +++ b/pkg/kubectl/cmd/get/get.go @@ -51,7 +51,7 @@ import ( // GetOptions contains the input to the get command. type GetOptions struct { PrintFlags *PrintFlags - ToPrinter func(*meta.RESTMapping, bool) (printers.ResourcePrinterFunc, error) + ToPrinter func(*meta.RESTMapping, bool, bool) (printers.ResourcePrinterFunc, error) IsHumanReadablePrinter bool PrintWithOpenAPICols bool @@ -224,10 +224,7 @@ func (o *GetOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []stri o.IncludeUninitialized = cmdutil.ShouldIncludeUninitialized(cmd, false) - if resource.MultipleTypesRequested(args) { - o.PrintFlags.EnsureWithKind() - } - o.ToPrinter = func(mapping *meta.RESTMapping, withNamespace bool) (printers.ResourcePrinterFunc, error) { + o.ToPrinter = func(mapping *meta.RESTMapping, withNamespace bool, withKind bool) (printers.ResourcePrinterFunc, error) { // make a new copy of current flags / opts before mutating printFlags := o.PrintFlags.Copy() @@ -242,6 +239,9 @@ func (o *GetOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []stri if withNamespace { printFlags.EnsureWithNamespace() } + if withKind { + printFlags.EnsureWithKind() + } printer, err := printFlags.ToPrinter() if err != nil { @@ -345,6 +345,7 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e if err != nil { allErrs = append(allErrs, err) } + printWithKind := multipleGVKsRequested(infos) objs := make([]runtime.Object, len(infos)) for ix := range infos { @@ -400,6 +401,7 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e nonEmptyObjCount++ printWithNamespace := o.AllNamespaces + if mapping != nil && mapping.Scope.Name() == meta.RESTScopeNameRoot { printWithNamespace = false } @@ -414,7 +416,7 @@ func (o *GetOptions) Run(f cmdutil.Factory, cmd *cobra.Command, args []string) e fmt.Fprintln(o.ErrOut) } - printer, err = o.ToPrinter(mapping, printWithNamespace) + printer, err = o.ToPrinter(mapping, printWithNamespace, printWithKind) if err != nil { if !errs.Has(err.Error()) { errs.Insert(err.Error()) @@ -493,30 +495,13 @@ func (o *GetOptions) watch(f cmdutil.Factory, cmd *cobra.Command, args []string) if err != nil { return err } - if len(infos) > 1 { - gvk := infos[0].Mapping.GroupVersionKind - uniqueGVKs := 1 - - // If requesting a resource count greater than a request's --chunk-size, - // we will end up making multiple requests to the server, with each - // request producing its own "Info" object. Although overall we are - // dealing with a single resource type, we will end up with multiple - // infos returned by the builder. To handle this case, only fail if we - // have at least one info with a different GVK than the others. - for _, info := range infos { - if info.Mapping.GroupVersionKind != gvk { - uniqueGVKs++ - } - } - - if uniqueGVKs > 1 { - return i18n.Errorf("watch is only supported on individual resources and resource collections - %d resources were found", uniqueGVKs) - } + if multipleGVKsRequested(infos) { + return i18n.Errorf("watch is only supported on individual resources and resource collections - more than 1 resources were found") } info := infos[0] mapping := info.ResourceMapping() - printer, err := o.ToPrinter(mapping, o.AllNamespaces) + printer, err := o.ToPrinter(mapping, o.AllNamespaces, false) if err != nil { return err } @@ -650,7 +635,7 @@ func (o *GetOptions) printGeneric(r *resource.Result) error { return utilerrors.Reduce(utilerrors.Flatten(utilerrors.NewAggregate(errs))) } - printer, err := o.ToPrinter(nil, false) + printer, err := o.ToPrinter(nil, false, false) if err != nil { return err } @@ -750,3 +735,16 @@ func maybeWrapSortingPrinter(printer printers.ResourcePrinter, sortBy string) pr } return printer } + +func multipleGVKsRequested(infos []*resource.Info) bool { + if len(infos) < 2 { + return false + } + gvk := infos[0].Mapping.GroupVersionKind + for _, info := range infos { + if info.Mapping.GroupVersionKind != gvk { + return true + } + } + return false +} diff --git a/pkg/kubectl/cmd/get/get_test.go b/pkg/kubectl/cmd/get/get_test.go index a6ab8990423..80f89670fc6 100644 --- a/pkg/kubectl/cmd/get/get_test.go +++ b/pkg/kubectl/cmd/get/get_test.go @@ -27,7 +27,12 @@ import ( "strings" "testing" + apiapps "k8s.io/api/apps/v1" + apiautoscaling "k8s.io/api/autoscaling/v1" + apibatchv1 "k8s.io/api/batch/v1" + apibatchv1beta1 "k8s.io/api/batch/v1beta1" api "k8s.io/api/core/v1" + apiextensionsv1beta1 "k8s.io/api/extensions/v1beta1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -351,6 +356,65 @@ pod/foo 0/0 0 } } +func TestGetMultipleResourceTypesShowKinds(t *testing.T) { + pods, svcs, _ := testData() + + tf := cmdtesting.NewTestFactory().WithNamespace("test") + defer tf.Cleanup() + codec := legacyscheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...) + + tf.UnstructuredClient = &fake.RESTClient{ + NegotiatedSerializer: unstructuredSerializer, + Resp: &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, &pods.Items[0])}, + } + tf.UnstructuredClient = &fake.RESTClient{ + NegotiatedSerializer: unstructuredSerializer, + Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { + switch p, m := req.URL.Path, req.Method; { + case p == "/namespaces/test/pods" && m == "GET": + return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, pods)}, nil + case p == "/namespaces/test/replicationcontrollers" && m == "GET": + return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, &api.ReplicationControllerList{})}, nil + case p == "/namespaces/test/services" && m == "GET": + return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, svcs)}, nil + case p == "/namespaces/test/statefulsets" && m == "GET": + return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, &apiapps.StatefulSetList{})}, nil + case p == "/namespaces/test/horizontalpodautoscalers" && m == "GET": + return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, &apiautoscaling.HorizontalPodAutoscalerList{})}, nil + case p == "/namespaces/test/jobs" && m == "GET": + return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, &apibatchv1.JobList{})}, nil + case p == "/namespaces/test/cronjobs" && m == "GET": + return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, &apibatchv1beta1.CronJobList{})}, nil + case p == "/namespaces/test/daemonsets" && m == "GET": + return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, &apiapps.DaemonSetList{})}, nil + case p == "/namespaces/test/deployments" && m == "GET": + return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, &apiextensionsv1beta1.DeploymentList{})}, nil + case p == "/namespaces/test/replicasets" && m == "GET": + return &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, &apiextensionsv1beta1.ReplicaSetList{})}, nil + + default: + t.Fatalf("request url: %#v,and request: %#v", req.URL, req) + return nil, nil + } + }), + } + + streams, _, buf, _ := genericclioptions.NewTestIOStreams() + cmd := NewCmdGet("kubectl", tf, streams) + cmd.SetOutput(buf) + cmd.Run(cmd, []string{"all"}) + + expected := `NAME READY STATUS RESTARTS AGE +pod/foo 0/0 0 +pod/bar 0/0 0 +NAME TYPE CLUSTER-IP EXTERNAL-IP PORT(S) AGE +service/baz ClusterIP +` + if e, a := expected, buf.String(); e != a { + t.Errorf("expected %v, got %v", e, a) + } +} + func TestGetObjectsShowLabels(t *testing.T) { pods, _, _ := testData() diff --git a/pkg/kubectl/cmd/testing/fake.go b/pkg/kubectl/cmd/testing/fake.go index 12d876626d7..4a4a87a7e68 100644 --- a/pkg/kubectl/cmd/testing/fake.go +++ b/pkg/kubectl/cmd/testing/fake.go @@ -501,6 +501,24 @@ func testDynamicResources() []*restmapper.APIGroupResources { }, }, }, + { + Group: metav1.APIGroup{ + Name: "batch", + Versions: []metav1.GroupVersionForDiscovery{ + {Version: "v1beta1"}, + {Version: "v1"}, + }, + PreferredVersion: metav1.GroupVersionForDiscovery{Version: "v1"}, + }, + VersionedResources: map[string][]metav1.APIResource{ + "v1beta1": { + {Name: "cronjobs", Namespaced: true, Kind: "CronJob"}, + }, + "v1": { + {Name: "jobs", Namespaced: true, Kind: "Job"}, + }, + }, + }, { Group: metav1.APIGroup{ Name: "autoscaling", diff --git a/pkg/kubectl/genericclioptions/resource/builder.go b/pkg/kubectl/genericclioptions/resource/builder.go index f2b82307350..b67a6976b4f 100644 --- a/pkg/kubectl/genericclioptions/resource/builder.go +++ b/pkg/kubectl/genericclioptions/resource/builder.go @@ -156,6 +156,8 @@ func newBuilder(clientConfigFn ClientConfigFunc, restMapper meta.RESTMapper, cat restMapper: restMapper, categoryExpander: categoryExpander, requireObject: true, + labelSelector: nil, + fieldSelector: nil, } } @@ -1107,37 +1109,3 @@ func HasNames(args []string) (bool, error) { } return hasCombinedTypes || len(args) > 1, nil } - -// MultipleTypesRequested returns true if the provided args contain multiple resource kinds -func MultipleTypesRequested(args []string) bool { - if len(args) == 1 && args[0] == "all" { - return true - } - - args = normalizeMultipleResourcesArgs(args) - rKinds := sets.NewString() - for _, arg := range args { - rTuple, found, err := splitResourceTypeName(arg) - if err != nil { - continue - } - - // if tuple not found, assume arg is of the form "type1,type2,...". - // Since SplitResourceArgument returns a unique list of kinds, - // return true here if len(uniqueList) > 1 - if !found { - if strings.Contains(arg, ",") { - splitArgs := SplitResourceArgument(arg) - if len(splitArgs) > 1 { - return true - } - } - continue - } - if rKinds.Has(rTuple.Resource) { - continue - } - rKinds.Insert(rTuple.Resource) - } - return rKinds.Len() > 1 -} diff --git a/pkg/kubectl/genericclioptions/resource/builder_test.go b/pkg/kubectl/genericclioptions/resource/builder_test.go index 93fbc3520e6..c6963e115d2 100644 --- a/pkg/kubectl/genericclioptions/resource/builder_test.go +++ b/pkg/kubectl/genericclioptions/resource/builder_test.go @@ -1396,80 +1396,3 @@ func TestHasNames(t *testing.T) { }) } } - -func TestMultipleTypesRequested(t *testing.T) { - tests := []struct { - name string - args []string - expectedMultipleTypes bool - }{ - { - name: "test1", - args: []string{""}, - expectedMultipleTypes: false, - }, - { - name: "test2", - args: []string{"all"}, - expectedMultipleTypes: true, - }, - { - name: "test3", - args: []string{"rc"}, - expectedMultipleTypes: false, - }, - { - name: "test4", - args: []string{"pod,all"}, - expectedMultipleTypes: true, - }, - { - name: "test5", - args: []string{"all,rc,pod"}, - expectedMultipleTypes: true, - }, - { - name: "test6", - args: []string{"rc,pod,svc"}, - expectedMultipleTypes: true, - }, - { - name: "test7", - args: []string{"rc/foo"}, - expectedMultipleTypes: false, - }, - { - name: "test8", - args: []string{"rc/foo", "rc/bar"}, - expectedMultipleTypes: false, - }, - { - name: "test9", - args: []string{"rc", "foo"}, - expectedMultipleTypes: false, - }, - { - name: "test10", - args: []string{"rc,pod,svc", "foo"}, - expectedMultipleTypes: true, - }, - { - name: "test11", - args: []string{"rc,secrets"}, - expectedMultipleTypes: true, - }, - { - name: "test12", - args: []string{"rc/foo", "rc/bar", "svc/svc"}, - expectedMultipleTypes: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - hasMultipleTypes := MultipleTypesRequested(tt.args) - if hasMultipleTypes != tt.expectedMultipleTypes { - t.Errorf("expected MultipleTypesRequested to return %v for %s", tt.expectedMultipleTypes, tt.args) - } - }) - } -}