From cf29a302582e7746c561e7365031ee6b5e79872f Mon Sep 17 00:00:00 2001 From: juanvallejo Date: Wed, 23 May 2018 17:43:04 -0400 Subject: [PATCH 1/2] move Describer from factory --- pkg/kubectl/cmd/describe.go | 5 +- pkg/kubectl/cmd/testing/fake.go | 7 +++ pkg/kubectl/cmd/util/factory.go | 3 -- .../cmd/util/factory_object_mapping.go | 44 ---------------- pkg/kubectl/cmd/util/helpers.go | 52 +++++++++++++++++++ 5 files changed, 63 insertions(+), 48 deletions(-) diff --git a/pkg/kubectl/cmd/describe.go b/pkg/kubectl/cmd/describe.go index 73dcd3dace3..848f82100f8 100644 --- a/pkg/kubectl/cmd/describe.go +++ b/pkg/kubectl/cmd/describe.go @@ -137,7 +137,10 @@ func (o *DescribeOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args [ o.BuilderArgs = args - o.Describer = f.Describer + o.Describer = func(mapping *meta.RESTMapping) (printers.Describer, error) { + return cmdutil.DescriberFn(f, mapping) + } + o.NewBuilder = f.NewBuilder // include the uninitialized objects by default diff --git a/pkg/kubectl/cmd/testing/fake.go b/pkg/kubectl/cmd/testing/fake.go index ce678c31665..59a4d947b58 100644 --- a/pkg/kubectl/cmd/testing/fake.go +++ b/pkg/kubectl/cmd/testing/fake.go @@ -271,10 +271,17 @@ func NewTestFactory() *TestFactory { WithClientConfig(clientConfig). WithRESTMapper(testRESTMapper()) + restConfig, err := clientConfig.ClientConfig() + if err != nil { + panic(fmt.Sprintf("unable to create a fake restclient config: %v", err)) + } + return &TestFactory{ Factory: cmdutil.NewFactory(configFlags), FakeDynamicClient: fakedynamic.NewSimpleDynamicClient(legacyscheme.Scheme), tempConfigFile: tmpFile, + + ClientConfigVal: restConfig, } } diff --git a/pkg/kubectl/cmd/util/factory.go b/pkg/kubectl/cmd/util/factory.go index 04e64f65ffa..10c50389346 100644 --- a/pkg/kubectl/cmd/util/factory.go +++ b/pkg/kubectl/cmd/util/factory.go @@ -35,7 +35,6 @@ import ( "k8s.io/kubernetes/pkg/kubectl/genericclioptions" "k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource" "k8s.io/kubernetes/pkg/kubectl/validation" - "k8s.io/kubernetes/pkg/printers" ) // Factory provides abstractions that allow the Kubectl command to be extended across multiple types @@ -102,8 +101,6 @@ type ObjectMappingFactory interface { ClientForMapping(mapping *meta.RESTMapping) (resource.RESTClient, error) // Returns a RESTClient for working with Unstructured objects. UnstructuredClientForMapping(mapping *meta.RESTMapping) (resource.RESTClient, error) - // Returns a Describer for displaying the specified RESTMapping type or an error. - Describer(mapping *meta.RESTMapping) (printers.Describer, error) // Returns a schema that can validate objects stored on disk. Validator(validate bool) (validation.Schema, error) diff --git a/pkg/kubectl/cmd/util/factory_object_mapping.go b/pkg/kubectl/cmd/util/factory_object_mapping.go index aa7105a1bd0..a4769978215 100644 --- a/pkg/kubectl/cmd/util/factory_object_mapping.go +++ b/pkg/kubectl/cmd/util/factory_object_mapping.go @@ -19,19 +19,15 @@ limitations under the License. package util import ( - "fmt" "sync" "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/client-go/dynamic" restclient "k8s.io/client-go/rest" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/kubectl/cmd/util/openapi" openapivalidation "k8s.io/kubernetes/pkg/kubectl/cmd/util/openapi/validation" "k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource" "k8s.io/kubernetes/pkg/kubectl/validation" - "k8s.io/kubernetes/pkg/printers" - printersinternal "k8s.io/kubernetes/pkg/printers/internalversion" ) type ring1Factory struct { @@ -91,46 +87,6 @@ func (f *ring1Factory) UnstructuredClientForMapping(mapping *meta.RESTMapping) ( return restclient.RESTClientFor(cfg) } -func (f *ring1Factory) Describer(mapping *meta.RESTMapping) (printers.Describer, error) { - clientConfig, err := f.clientAccessFactory.ToRESTConfig() - if err != nil { - return nil, err - } - // try to get a describer - if describer, ok := printersinternal.DescriberFor(mapping.GroupVersionKind.GroupKind(), clientConfig); ok { - return describer, nil - } - // if this is a kind we don't have a describer for yet, go generic if possible - if genericDescriber, genericErr := genericDescriber(f.clientAccessFactory, mapping); genericErr == nil { - return genericDescriber, nil - } - // otherwise return an unregistered error - return nil, fmt.Errorf("no description has been implemented for %s", mapping.GroupVersionKind.String()) -} - -// helper function to make a generic describer, or return an error -func genericDescriber(clientAccessFactory ClientAccessFactory, mapping *meta.RESTMapping) (printers.Describer, error) { - clientConfig, err := clientAccessFactory.ToRESTConfig() - if err != nil { - return nil, err - } - - // used to fetch the resource - dynamicClient, err := dynamic.NewForConfig(clientConfig) - if err != nil { - return nil, err - } - - // used to get events for the resource - clientSet, err := clientAccessFactory.ClientSet() - if err != nil { - return nil, err - } - eventsClient := clientSet.Core() - - return printersinternal.GenericDescriberFor(mapping, dynamicClient, eventsClient), nil -} - func (f *ring1Factory) Validator(validate bool) (validation.Schema, error) { if !validate { return validation.NullSchema{}, nil diff --git a/pkg/kubectl/cmd/util/helpers.go b/pkg/kubectl/cmd/util/helpers.go index 1e5aa7dc6a1..b4640f67b25 100644 --- a/pkg/kubectl/cmd/util/helpers.go +++ b/pkg/kubectl/cmd/util/helpers.go @@ -37,9 +37,14 @@ import ( utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/yaml" + "k8s.io/client-go/dynamic" "k8s.io/client-go/tools/clientcmd" + "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" "k8s.io/kubernetes/pkg/kubectl" + "k8s.io/kubernetes/pkg/kubectl/genericclioptions" "k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource" + "k8s.io/kubernetes/pkg/printers" + printersinternal "k8s.io/kubernetes/pkg/printers/internalversion" utilexec "k8s.io/utils/exec" ) @@ -621,3 +626,50 @@ func ShouldIncludeUninitialized(cmd *cobra.Command, includeUninitialized bool) b } return shouldIncludeUninitialized } + +// DescriberFunc gives a way to display the specified RESTMapping type +type DescriberFunc func(restClientGetter genericclioptions.RESTClientGetter, mapping *meta.RESTMapping) (printers.Describer, error) + +// DescriberFn gives a way to easily override the function for unit testing if needed +var DescriberFn DescriberFunc = describer + +// Returns a Describer for displaying the specified RESTMapping type or an error. +func describer(restClientGetter genericclioptions.RESTClientGetter, mapping *meta.RESTMapping) (printers.Describer, error) { + clientConfig, err := restClientGetter.ToRESTConfig() + if err != nil { + return nil, err + } + // try to get a describer + if describer, ok := printersinternal.DescriberFor(mapping.GroupVersionKind.GroupKind(), clientConfig); ok { + return describer, nil + } + // if this is a kind we don't have a describer for yet, go generic if possible + if genericDescriber, genericErr := genericDescriber(restClientGetter, mapping); genericErr == nil { + return genericDescriber, nil + } + // otherwise return an unregistered error + return nil, fmt.Errorf("no description has been implemented for %s", mapping.GroupVersionKind.String()) +} + +// helper function to make a generic describer, or return an error +func genericDescriber(restClientGetter genericclioptions.RESTClientGetter, mapping *meta.RESTMapping) (printers.Describer, error) { + clientConfig, err := restClientGetter.ToRESTConfig() + if err != nil { + return nil, err + } + + // used to fetch the resource + dynamicClient, err := dynamic.NewForConfig(clientConfig) + if err != nil { + return nil, err + } + + // used to get events for the resource + clientSet, err := internalclientset.NewForConfig(clientConfig) + if err != nil { + return nil, err + } + + eventsClient := clientSet.Core() + return printersinternal.GenericDescriberFor(mapping, dynamicClient, eventsClient), nil +} From 6d117383fc575bfb084c00a11b22ec97b279f490 Mon Sep 17 00:00:00 2001 From: David Eads Date: Thu, 24 May 2018 10:50:49 -0400 Subject: [PATCH 2/2] fix describer tests --- pkg/kubectl/cmd/cmd_test.go | 14 ------- pkg/kubectl/cmd/describe_test.go | 70 +++++++++++++++++++++++++++----- pkg/kubectl/cmd/testing/BUILD | 1 - pkg/kubectl/cmd/testing/fake.go | 6 --- 4 files changed, 60 insertions(+), 31 deletions(-) diff --git a/pkg/kubectl/cmd/cmd_test.go b/pkg/kubectl/cmd/cmd_test.go index 742d8926841..8ae9c605401 100644 --- a/pkg/kubectl/cmd/cmd_test.go +++ b/pkg/kubectl/cmd/cmd_test.go @@ -39,7 +39,6 @@ import ( api "k8s.io/kubernetes/pkg/apis/core" cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util" "k8s.io/kubernetes/pkg/kubectl/scheme" - "k8s.io/kubernetes/pkg/printers" ) // This init should be removed after switching this command and its tests to user external types. @@ -116,19 +115,6 @@ func testData() (*api.PodList, *api.ServiceList, *api.ReplicationControllerList) return pods, svc, rc } -type testDescriber struct { - Name, Namespace string - Settings printers.DescriberSettings - Output string - Err error -} - -func (t *testDescriber) Describe(namespace, name string, describerSettings printers.DescriberSettings) (output string, err error) { - t.Namespace, t.Name = namespace, name - t.Settings = describerSettings - return t.Output, t.Err -} - func objBody(codec runtime.Codec, obj runtime.Object) io.ReadCloser { return ioutil.NopCloser(bytes.NewReader([]byte(runtime.EncodeOrDie(codec, obj)))) } diff --git a/pkg/kubectl/cmd/describe_test.go b/pkg/kubectl/cmd/describe_test.go index 6ecc1eb909d..ecc0fc8c4ed 100644 --- a/pkg/kubectl/cmd/describe_test.go +++ b/pkg/kubectl/cmd/describe_test.go @@ -22,20 +22,29 @@ import ( "strings" "testing" + "k8s.io/apimachinery/pkg/api/meta" "k8s.io/client-go/rest/fake" "k8s.io/kubernetes/pkg/api/legacyscheme" cmdtesting "k8s.io/kubernetes/pkg/kubectl/cmd/testing" + cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util" "k8s.io/kubernetes/pkg/kubectl/genericclioptions" "k8s.io/kubernetes/pkg/kubectl/scheme" + "k8s.io/kubernetes/pkg/printers" ) // Verifies that schemas that are not in the master tree of Kubernetes can be retrieved via Get. func TestDescribeUnknownSchemaObject(t *testing.T) { d := &testDescriber{Output: "test output"} + oldFn := cmdutil.DescriberFn + defer func() { + cmdutil.DescriberFn = oldFn + }() + cmdutil.DescriberFn = d.describerFor + tf := cmdtesting.NewTestFactory() defer tf.Cleanup() _, _, codec := cmdtesting.NewExternalScheme() - tf.DescriberVal = d + tf.UnstructuredClient = &fake.RESTClient{ NegotiatedSerializer: unstructuredSerializer, Resp: &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, cmdtesting.NewInternalType("", "", "foo"))}, @@ -59,11 +68,16 @@ func TestDescribeUnknownSchemaObject(t *testing.T) { // Verifies that schemas that are not in the master tree of Kubernetes can be retrieved via Get. func TestDescribeUnknownNamespacedSchemaObject(t *testing.T) { d := &testDescriber{Output: "test output"} + oldFn := cmdutil.DescriberFn + defer func() { + cmdutil.DescriberFn = oldFn + }() + cmdutil.DescriberFn = d.describerFor + tf := cmdtesting.NewTestFactory() defer tf.Cleanup() _, _, codec := cmdtesting.NewExternalScheme() - tf.DescriberVal = d tf.UnstructuredClient = &fake.RESTClient{ NegotiatedSerializer: unstructuredSerializer, Resp: &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, cmdtesting.NewInternalNamespacedType("", "", "foo", "non-default"))}, @@ -85,13 +99,18 @@ func TestDescribeUnknownNamespacedSchemaObject(t *testing.T) { } func TestDescribeObject(t *testing.T) { + d := &testDescriber{Output: "test output"} + oldFn := cmdutil.DescriberFn + defer func() { + cmdutil.DescriberFn = oldFn + }() + cmdutil.DescriberFn = d.describerFor + _, _, rc := testData() tf := cmdtesting.NewTestFactory() defer tf.Cleanup() codec := legacyscheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...) - d := &testDescriber{Output: "test output"} - tf.DescriberVal = d tf.UnstructuredClient = &fake.RESTClient{ NegotiatedSerializer: unstructuredSerializer, Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) { @@ -122,13 +141,18 @@ func TestDescribeObject(t *testing.T) { } func TestDescribeListObjects(t *testing.T) { + d := &testDescriber{Output: "test output"} + oldFn := cmdutil.DescriberFn + defer func() { + cmdutil.DescriberFn = oldFn + }() + cmdutil.DescriberFn = d.describerFor + pods, _, _ := testData() tf := cmdtesting.NewTestFactory() defer tf.Cleanup() codec := legacyscheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...) - d := &testDescriber{Output: "test output"} - tf.DescriberVal = d tf.UnstructuredClient = &fake.RESTClient{ NegotiatedSerializer: unstructuredSerializer, Resp: &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, pods)}, @@ -145,13 +169,18 @@ func TestDescribeListObjects(t *testing.T) { } func TestDescribeObjectShowEvents(t *testing.T) { + d := &testDescriber{Output: "test output"} + oldFn := cmdutil.DescriberFn + defer func() { + cmdutil.DescriberFn = oldFn + }() + cmdutil.DescriberFn = d.describerFor + pods, _, _ := testData() tf := cmdtesting.NewTestFactory() defer tf.Cleanup() codec := legacyscheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...) - d := &testDescriber{Output: "test output"} - tf.DescriberVal = d tf.UnstructuredClient = &fake.RESTClient{ NegotiatedSerializer: unstructuredSerializer, Resp: &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, pods)}, @@ -167,13 +196,18 @@ func TestDescribeObjectShowEvents(t *testing.T) { } func TestDescribeObjectSkipEvents(t *testing.T) { + d := &testDescriber{Output: "test output"} + oldFn := cmdutil.DescriberFn + defer func() { + cmdutil.DescriberFn = oldFn + }() + cmdutil.DescriberFn = d.describerFor + pods, _, _ := testData() tf := cmdtesting.NewTestFactory() defer tf.Cleanup() codec := legacyscheme.Codecs.LegacyCodec(scheme.Scheme.PrioritizedVersionsAllGroups()...) - d := &testDescriber{Output: "test output"} - tf.DescriberVal = d tf.UnstructuredClient = &fake.RESTClient{ NegotiatedSerializer: unstructuredSerializer, Resp: &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, pods)}, @@ -215,3 +249,19 @@ func TestDescribeHelpMessage(t *testing.T) { t.Errorf("Expected not to contain: \n %v\nGot:\n %v\n", unexpected, got) } } + +type testDescriber struct { + Name, Namespace string + Settings printers.DescriberSettings + Output string + Err error +} + +func (t *testDescriber) Describe(namespace, name string, describerSettings printers.DescriberSettings) (output string, err error) { + t.Namespace, t.Name = namespace, name + t.Settings = describerSettings + return t.Output, t.Err +} +func (t *testDescriber) describerFor(restClientGetter genericclioptions.RESTClientGetter, mapping *meta.RESTMapping) (printers.Describer, error) { + return t, nil +} diff --git a/pkg/kubectl/cmd/testing/BUILD b/pkg/kubectl/cmd/testing/BUILD index 673c74c85e1..8c615c2cf7f 100644 --- a/pkg/kubectl/cmd/testing/BUILD +++ b/pkg/kubectl/cmd/testing/BUILD @@ -18,7 +18,6 @@ go_library( "//pkg/kubectl/genericclioptions:go_default_library", "//pkg/kubectl/genericclioptions/resource:go_default_library", "//pkg/kubectl/validation:go_default_library", - "//pkg/printers:go_default_library", "//vendor/github.com/spf13/cobra:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library", "//vendor/k8s.io/apimachinery/pkg/api/meta/testrestmapper:go_default_library", diff --git a/pkg/kubectl/cmd/testing/fake.go b/pkg/kubectl/cmd/testing/fake.go index 59a4d947b58..839ee29dbea 100644 --- a/pkg/kubectl/cmd/testing/fake.go +++ b/pkg/kubectl/cmd/testing/fake.go @@ -52,7 +52,6 @@ import ( "k8s.io/kubernetes/pkg/kubectl/genericclioptions" "k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource" "k8s.io/kubernetes/pkg/kubectl/validation" - "k8s.io/kubernetes/pkg/printers" ) // +k8s:deepcopy-gen=true @@ -238,7 +237,6 @@ type TestFactory struct { Client kubectl.RESTClient ScaleGetter scaleclient.ScalesGetter UnstructuredClient kubectl.RESTClient - DescriberVal printers.Describer Namespace string ClientConfigVal *restclient.Config CommandVal string @@ -312,10 +310,6 @@ func (f *TestFactory) UnstructuredClientForMapping(mapping *meta.RESTMapping) (r return f.UnstructuredClient, nil } -func (f *TestFactory) Describer(*meta.RESTMapping) (printers.Describer, error) { - return f.DescriberVal, nil -} - func (f *TestFactory) Validator(validate bool) (validation.Schema, error) { return validation.NullSchema{}, nil }