Merge pull request #64328 from deads2k/cli-70-onefactory

Automatic merge from submit-queue (batch tested with PRs 64355, 64328, 64352). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

collapse into one factory

We've stripped enough out of the factory that we can strip down to a single factory instead of ringed set.


@kubernetes/sig-cli-maintainers 

Nothing else should be added to the factory.  Instead we should build particular constructors atop the RESTClientGetter and structure commands with clear wiring stages and execution stages to simplify the unit testing for each piece.  See https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/wait/wait.go  .  It is easily unit testable with "standard" mocks and constructs as compared to using the factory as an injector an retesting a builder to act as a surrogate API server using a different client mocking mechanism.

@soltysh @juanvallejo I'm fine with this after your currently open snips.

```release-note
NONE
```

pkg/kubectl/cmd/util/BUILD

@@ -5,9 +5,7 @@ go_library(
     srcs = [
         "conversion.go",
         "factory.go",
-        "factory_builder.go",
         "factory_client_access.go",
-        "factory_object_mapping.go",
         "helpers.go",
         "kubectl_match_version.go",
         "printing.go",

pkg/kubectl/cmd/util/factory.go

@@ -46,15 +46,6 @@ import (
 // TODO: pass the various interfaces on the factory directly into the command constructors (so the
 // commands are decoupled from the factory).
 type Factory interface {
-	ClientAccessFactory
-	ObjectMappingFactory
-	BuilderFactory
-}
-
-// ClientAccessFactory holds the first level of factory methods.
-// Generally provides discovery, negotiation, and no-dep calls.
-// TODO The polymorphic calls probably deserve their own interface.
-type ClientAccessFactory interface {
 	genericclioptions.RESTClientGetter
 
 	// ClientSet gives you back an internal, generated clientset
@@ -79,11 +70,7 @@ type ClientAccessFactory interface {
 	DefaultNamespace() (string, bool, error)
 	// Generators returns the generators for the provided command
 	Generators(cmdName string) map[string]kubectl.Generator
-}
 
-// ObjectMappingFactory holds the second level of factory methods. These functions depend upon ClientAccessFactory methods.
-// Generally they provide object typing and functions that build requests based on the negotiated clients.
-type ObjectMappingFactory interface {
 	// Returns a RESTClient for working with the specified RESTMapping or an error. This is intended
 	// for working with arbitrary resources and is not guaranteed to point to a Kubernetes APIServer.
 	ClientForMapping(mapping *meta.RESTMapping) (resource.RESTClient, error)
@@ -94,35 +81,11 @@ type ObjectMappingFactory interface {
 	Validator(validate bool) (validation.Schema, error)
 	// OpenAPISchema returns the schema openapi schema definition
 	OpenAPISchema() (openapi.Resources, error)
-}
 
-// BuilderFactory holds the third level of factory methods. These functions depend upon ObjectMappingFactory and ClientAccessFactory methods.
-// Generally they depend upon client mapper functions
-type BuilderFactory interface {
 	// ScaleClient gives you back scale getter
 	ScaleClient() (scaleclient.ScalesGetter, error)
 }
 
-type factory struct {
-	ClientAccessFactory
-	ObjectMappingFactory
-	BuilderFactory
-}
-
-// NewFactory creates a factory with the default Kubernetes resources defined
-// Receives a clientGetter capable of providing a discovery client and a REST client configuration.
-func NewFactory(clientGetter genericclioptions.RESTClientGetter) Factory {
-	clientAccessFactory := NewClientAccessFactory(clientGetter)
-	objectMappingFactory := NewObjectMappingFactory(clientAccessFactory)
-	builderFactory := NewBuilderFactory(clientAccessFactory, objectMappingFactory)
-
-	return &factory{
-		ClientAccessFactory:  clientAccessFactory,
-		ObjectMappingFactory: objectMappingFactory,
-		BuilderFactory:       builderFactory,
-	}
-}
-
 func makePortsString(ports []api.ServicePort, useNodePort bool) string {
 	pieces := make([]string, len(ports))
 	for ix := range ports {

pkg/kubectl/cmd/util/factory_builder.go

@@ -1,56 +0,0 @@
-/*
-Copyright 2016 The Kubernetes Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-// this file contains factories with no other dependencies
-
-package util
-
-import (
-	"k8s.io/client-go/dynamic"
-	scaleclient "k8s.io/client-go/scale"
-)
-
-type ring2Factory struct {
-	clientAccessFactory  ClientAccessFactory
-	objectMappingFactory ObjectMappingFactory
-}
-
-func NewBuilderFactory(clientAccessFactory ClientAccessFactory, objectMappingFactory ObjectMappingFactory) BuilderFactory {
-	f := &ring2Factory{
-		clientAccessFactory:  clientAccessFactory,
-		objectMappingFactory: objectMappingFactory,
-	}
-
-	return f
-}
-
-func (f *ring2Factory) ScaleClient() (scaleclient.ScalesGetter, error) {
-	discoClient, err := f.clientAccessFactory.ToDiscoveryClient()
-	if err != nil {
-		return nil, err
-	}
-	restClient, err := f.clientAccessFactory.RESTClient()
-	if err != nil {
-		return nil, err
-	}
-	resolver := scaleclient.NewDiscoveryScaleKindResolver(discoClient)
-	mapper, err := f.clientAccessFactory.ToRESTMapper()
-	if err != nil {
-		return nil, err
-	}
-
-	return scaleclient.New(restClient, mapper, dynamic.LegacyAPIPathResolverFunc, resolver), nil
-}

pkg/kubectl/cmd/util/factory_client_access.go

@@ -21,6 +21,7 @@ package util
 import (
 	"fmt"
 	"io"
+	"sync"
 
 	appsv1 "k8s.io/api/apps/v1"
 	appsv1beta1 "k8s.io/api/apps/v1beta1"
@@ -38,47 +39,60 @@ import (
 	"k8s.io/client-go/dynamic"
 	"k8s.io/client-go/kubernetes"
 	restclient "k8s.io/client-go/rest"
+	scaleclient "k8s.io/client-go/scale"
 	"k8s.io/client-go/tools/clientcmd"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
+	api "k8s.io/kubernetes/pkg/apis/core"
 	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 	"k8s.io/kubernetes/pkg/kubectl"
+	"k8s.io/kubernetes/pkg/kubectl/cmd/util/openapi"
+	openapivalidation "k8s.io/kubernetes/pkg/kubectl/cmd/util/openapi/validation"
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions"
 	"k8s.io/kubernetes/pkg/kubectl/genericclioptions/resource"
+	"k8s.io/kubernetes/pkg/kubectl/validation"
 )
 
-type ring0Factory struct {
+type factoryImpl struct {
 	clientGetter genericclioptions.RESTClientGetter
+
+	// openAPIGetter loads and caches openapi specs
+	openAPIGetter openAPIGetter
 }
 
-func NewClientAccessFactory(clientGetter genericclioptions.RESTClientGetter) ClientAccessFactory {
+type openAPIGetter struct {
+	once   sync.Once
+	getter openapi.Getter
+}
+
+func NewFactory(clientGetter genericclioptions.RESTClientGetter) Factory {
 	if clientGetter == nil {
 		panic("attempt to instantiate client_access_factory with nil clientGetter")
 	}
 
-	f := &ring0Factory{
+	f := &factoryImpl{
 		clientGetter: clientGetter,
 	}
 
 	return f
 }
 
-func (f *ring0Factory) ToRESTConfig() (*restclient.Config, error) {
+func (f *factoryImpl) ToRESTConfig() (*restclient.Config, error) {
 	return f.clientGetter.ToRESTConfig()
 }
 
-func (f *ring0Factory) ToRESTMapper() (meta.RESTMapper, error) {
+func (f *factoryImpl) ToRESTMapper() (meta.RESTMapper, error) {
 	return f.clientGetter.ToRESTMapper()
 }
 
-func (f *ring0Factory) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) {
+func (f *factoryImpl) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) {
 	return f.clientGetter.ToDiscoveryClient()
 }
 
-func (f *ring0Factory) ToRawKubeConfigLoader() clientcmd.ClientConfig {
+func (f *factoryImpl) ToRawKubeConfigLoader() clientcmd.ClientConfig {
 	return f.clientGetter.ToRawKubeConfigLoader()
 }
 
-func (f *ring0Factory) KubernetesClientSet() (*kubernetes.Clientset, error) {
+func (f *factoryImpl) KubernetesClientSet() (*kubernetes.Clientset, error) {
 	clientConfig, err := f.ToRESTConfig()
 	if err != nil {
 		return nil, err
@@ -86,7 +100,7 @@ func (f *ring0Factory) KubernetesClientSet() (*kubernetes.Clientset, error) {
 	return kubernetes.NewForConfig(clientConfig)
 }
 
-func (f *ring0Factory) ClientSet() (internalclientset.Interface, error) {
+func (f *factoryImpl) ClientSet() (internalclientset.Interface, error) {
 	clientConfig, err := f.ToRESTConfig()
 	if err != nil {
 		return nil, err
@@ -94,7 +108,7 @@ func (f *ring0Factory) ClientSet() (internalclientset.Interface, error) {
 	return internalclientset.NewForConfig(clientConfig)
 }
 
-func (f *ring0Factory) DynamicClient() (dynamic.Interface, error) {
+func (f *factoryImpl) DynamicClient() (dynamic.Interface, error) {
 	clientConfig, err := f.ToRESTConfig()
 	if err != nil {
 		return nil, err
@@ -103,11 +117,11 @@ func (f *ring0Factory) DynamicClient() (dynamic.Interface, error) {
 }
 
 // NewBuilder returns a new resource builder for structured api objects.
-func (f *ring0Factory) NewBuilder() *resource.Builder {
+func (f *factoryImpl) NewBuilder() *resource.Builder {
 	return resource.NewBuilder(f.clientGetter)
 }
 
-func (f *ring0Factory) RESTClient() (*restclient.RESTClient, error) {
+func (f *factoryImpl) RESTClient() (*restclient.RESTClient, error) {
 	clientConfig, err := f.ToRESTConfig()
 	if err != nil {
 		return nil, err
@@ -116,10 +130,99 @@ func (f *ring0Factory) RESTClient() (*restclient.RESTClient, error) {
 	return restclient.RESTClientFor(clientConfig)
 }
 
-func (f *ring0Factory) DefaultNamespace() (string, bool, error) {
+func (f *factoryImpl) DefaultNamespace() (string, bool, error) {
 	return f.clientGetter.ToRawKubeConfigLoader().Namespace()
 }
 
+func (f *factoryImpl) ClientForMapping(mapping *meta.RESTMapping) (resource.RESTClient, error) {
+	cfg, err := f.clientGetter.ToRESTConfig()
+	if err != nil {
+		return nil, err
+	}
+	if err := setKubernetesDefaults(cfg); err != nil {
+		return nil, err
+	}
+	gvk := mapping.GroupVersionKind
+	switch gvk.Group {
+	case api.GroupName:
+		cfg.APIPath = "/api"
+	default:
+		cfg.APIPath = "/apis"
+	}
+	gv := gvk.GroupVersion()
+	cfg.GroupVersion = &gv
+	return restclient.RESTClientFor(cfg)
+}
+
+func (f *factoryImpl) UnstructuredClientForMapping(mapping *meta.RESTMapping) (resource.RESTClient, error) {
+	cfg, err := f.clientGetter.ToRESTConfig()
+	if err != nil {
+		return nil, err
+	}
+	if err := restclient.SetKubernetesDefaults(cfg); err != nil {
+		return nil, err
+	}
+	cfg.APIPath = "/apis"
+	if mapping.GroupVersionKind.Group == api.GroupName {
+		cfg.APIPath = "/api"
+	}
+	gv := mapping.GroupVersionKind.GroupVersion()
+	cfg.ContentConfig = resource.UnstructuredPlusDefaultContentConfig()
+	cfg.GroupVersion = &gv
+	return restclient.RESTClientFor(cfg)
+}
+
+func (f *factoryImpl) Validator(validate bool) (validation.Schema, error) {
+	if !validate {
+		return validation.NullSchema{}, nil
+	}
+
+	resources, err := f.OpenAPISchema()
+	if err != nil {
+		return nil, err
+	}
+
+	return validation.ConjunctiveSchema{
+		openapivalidation.NewSchemaValidation(resources),
+		validation.NoDoubleKeySchema{},
+	}, nil
+}
+
+// OpenAPISchema returns metadata and structural information about Kubernetes object definitions.
+func (f *factoryImpl) OpenAPISchema() (openapi.Resources, error) {
+	discovery, err := f.clientGetter.ToDiscoveryClient()
+	if err != nil {
+		return nil, err
+	}
+
+	// Lazily initialize the OpenAPIGetter once
+	f.openAPIGetter.once.Do(func() {
+		// Create the caching OpenAPIGetter
+		f.openAPIGetter.getter = openapi.NewOpenAPIGetter(discovery)
+	})
+
+	// Delegate to the OpenAPIGetter
+	return f.openAPIGetter.getter.Get()
+}
+
+func (f *factoryImpl) ScaleClient() (scaleclient.ScalesGetter, error) {
+	discoClient, err := f.clientGetter.ToDiscoveryClient()
+	if err != nil {
+		return nil, err
+	}
+	restClient, err := f.RESTClient()
+	if err != nil {
+		return nil, err
+	}
+	resolver := scaleclient.NewDiscoveryScaleKindResolver(discoClient)
+	mapper, err := f.clientGetter.ToRESTMapper()
+	if err != nil {
+		return nil, err
+	}
+
+	return scaleclient.New(restClient, mapper, dynamic.LegacyAPIPathResolverFunc, resolver), nil
+}
+
 const (
 	// TODO(sig-cli): Enforce consistent naming for generators here.
 	// See discussion in https://github.com/kubernetes/kubernetes/issues/46237
@@ -326,7 +429,7 @@ func Contains(resourcesList []*metav1.APIResourceList, resource schema.GroupVers
 	return len(resources) != 0
 }
 
-func (f *ring0Factory) Generators(cmdName string) map[string]kubectl.Generator {
+func (f *factoryImpl) Generators(cmdName string) map[string]kubectl.Generator {
 	return DefaultGenerators(cmdName)
 }
 

pkg/kubectl/cmd/util/factory_object_mapping.go

@@ -1,121 +0,0 @@
-/*
-Copyright 2016 The Kubernetes Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-// this file contains factories with no other dependencies
-
-package util
-
-import (
-	"sync"
-
-	"k8s.io/apimachinery/pkg/api/meta"
-	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"
-)
-
-type ring1Factory struct {
-	clientAccessFactory ClientAccessFactory
-
-	// openAPIGetter loads and caches openapi specs
-	openAPIGetter openAPIGetter
-}
-
-type openAPIGetter struct {
-	once   sync.Once
-	getter openapi.Getter
-}
-
-func NewObjectMappingFactory(clientAccessFactory ClientAccessFactory) ObjectMappingFactory {
-	f := &ring1Factory{
-		clientAccessFactory: clientAccessFactory,
-	}
-	return f
-}
-
-func (f *ring1Factory) ClientForMapping(mapping *meta.RESTMapping) (resource.RESTClient, error) {
-	cfg, err := f.clientAccessFactory.ToRESTConfig()
-	if err != nil {
-		return nil, err
-	}
-	if err := setKubernetesDefaults(cfg); err != nil {
-		return nil, err
-	}
-	gvk := mapping.GroupVersionKind
-	switch gvk.Group {
-	case api.GroupName:
-		cfg.APIPath = "/api"
-	default:
-		cfg.APIPath = "/apis"
-	}
-	gv := gvk.GroupVersion()
-	cfg.GroupVersion = &gv
-	return restclient.RESTClientFor(cfg)
-}
-
-func (f *ring1Factory) UnstructuredClientForMapping(mapping *meta.RESTMapping) (resource.RESTClient, error) {
-	cfg, err := f.clientAccessFactory.ToRESTConfig()
-	if err != nil {
-		return nil, err
-	}
-	if err := restclient.SetKubernetesDefaults(cfg); err != nil {
-		return nil, err
-	}
-	cfg.APIPath = "/apis"
-	if mapping.GroupVersionKind.Group == api.GroupName {
-		cfg.APIPath = "/api"
-	}
-	gv := mapping.GroupVersionKind.GroupVersion()
-	cfg.ContentConfig = resource.UnstructuredPlusDefaultContentConfig()
-	cfg.GroupVersion = &gv
-	return restclient.RESTClientFor(cfg)
-}
-
-func (f *ring1Factory) Validator(validate bool) (validation.Schema, error) {
-	if !validate {
-		return validation.NullSchema{}, nil
-	}
-
-	resources, err := f.OpenAPISchema()
-	if err != nil {
-		return nil, err
-	}
-
-	return validation.ConjunctiveSchema{
-		openapivalidation.NewSchemaValidation(resources),
-		validation.NoDoubleKeySchema{},
-	}, nil
-}
-
-// OpenAPISchema returns metadata and structural information about Kubernetes object definitions.
-func (f *ring1Factory) OpenAPISchema() (openapi.Resources, error) {
-	discovery, err := f.clientAccessFactory.ToDiscoveryClient()
-	if err != nil {
-		return nil, err
-	}
-
-	// Lazily initialize the OpenAPIGetter once
-	f.openAPIGetter.once.Do(func() {
-		// Create the caching OpenAPIGetter
-		f.openAPIGetter.getter = openapi.NewOpenAPIGetter(discovery)
-	})
-
-	// Delegate to the OpenAPIGetter
-	return f.openAPIGetter.getter.Get()
-}