Merge pull request #64233 from juanvallejo/jvallejo/move-describer-out-of-factory

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))))
 }

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

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
+}

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",

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
@@ -271,10 +269,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,
 	}
 }
 
@@ -305,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
 }

pkg/kubectl/cmd/util/factory.go

@@ -34,7 +34,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
@@ -94,8 +93,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)

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

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
+}