Merge pull request #47675 from alexandercampbell/refactor-long-kubectl-function

Automatic merge from submit-queue (batch tested with PRs 47675, 48001)

cmd/create_deployment: refactor long function

Refactor the `createDeployment` function under `pkg/kubectl/cmd`.

- [x] Behavior has been extracted to two helper functions.
- [x] Behavior remains identical.
- [x] Logic has been made explicit through function naming and comments.

This is essentially the pattern I've been following in my larger branches (the ones that are pending the merge of #46468). Want to get some design feedback before I get too far away from `master`.

Thanks!

cc @apelisse @mengqiy @droot 

**Release note**:

```release-note
NONE
```

pkg/kubectl/cmd/create_deployment.go

@@ -23,6 +23,7 @@ import (
 	"github.com/spf13/cobra"
 
 	appsv1beta1 "k8s.io/api/apps/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/kubernetes/pkg/kubectl"
 	"k8s.io/kubernetes/pkg/kubectl/cmd/templates"
 	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
@@ -31,11 +32,11 @@ import (
 
 var (
 	deploymentLong = templates.LongDesc(i18n.T(`
-    Create a deployment with the specified name.`))
+	Create a deployment with the specified name.`))
 
 	deploymentExample = templates.Examples(i18n.T(`
-    # Create a new deployment named my-dep that runs the busybox image.
-    kubectl create deployment my-dep --image=busybox`))
+	# Create a new deployment named my-dep that runs the busybox image.
+	kubectl create deployment my-dep --image=busybox`))
 )
 
 // NewCmdCreateDeployment is a macro command to create a new deployment.
@@ -62,8 +63,69 @@ func NewCmdCreateDeployment(f cmdutil.Factory, cmdOut, cmdErr io.Writer) *cobra.
 	return cmd
 }
 
-func createDeployment(f cmdutil.Factory, cmdOut, cmdErr io.Writer, cmd *cobra.Command, args []string) error {
-	name, err := NameFromCommandArgs(cmd, args)
+// fallbackGeneratorNameIfNecessary returns the name of the old generator
+// (v1beta1) if server does not support apps/v1beta1 deployments. Otherwise, the
+// generator string is returned unchanged.
+//
+// If the generator name is changed, print a warning message to let the user
+// know.
+func fallbackGeneratorNameIfNecessary(
+	generatorName string,
+	resourcesList []*metav1.APIResourceList,
+	cmdErr io.Writer,
+) string {
+
+	if generatorName == cmdutil.DeploymentBasicAppsV1Beta1GeneratorName &&
+		!contains(resourcesList, appsv1beta1.SchemeGroupVersion.WithResource("deployments")) {
+
+		fmt.Fprintf(cmdErr,
+			"WARNING: New deployments generator %q specified, "+
+				"but apps/v1beta1.Deployments are not available. "+
+				"Falling back to %q.\n",
+			cmdutil.DeploymentBasicAppsV1Beta1GeneratorName,
+			cmdutil.DeploymentBasicV1Beta1GeneratorName,
+		)
+
+		return cmdutil.DeploymentBasicV1Beta1GeneratorName
+	}
+
+	return generatorName
+}
+
+// generatorFromName returns the appropriate StructuredGenerator based on the
+// generatorName. If the generatorName is unrecognized, then return (nil,
+// false).
+func generatorFromName(
+	generatorName string,
+	imageNames []string,
+	deploymentName string,
+) (kubectl.StructuredGenerator, bool) {
+
+	switch generatorName {
+	case cmdutil.DeploymentBasicAppsV1Beta1GeneratorName:
+		return &kubectl.DeploymentBasicAppsGeneratorV1{
+			Name:   deploymentName,
+			Images: imageNames,
+		}, true
+
+	case cmdutil.DeploymentBasicV1Beta1GeneratorName:
+		return &kubectl.DeploymentBasicGeneratorV1{
+			Name:   deploymentName,
+			Images: imageNames,
+		}, true
+	}
+
+	return nil, false
+}
+
+// createDeployment
+// 1. Reads user config values from Cobra.
+// 2. Sets up the correct Generator object.
+// 3. Calls RunCreateSubcommand.
+func createDeployment(f cmdutil.Factory, cmdOut, cmdErr io.Writer,
+	cmd *cobra.Command, args []string) error {
+
+	deploymentName, err := NameFromCommandArgs(cmd, args)
 	if err != nil {
 		return err
 	}
@@ -77,25 +139,21 @@ func createDeployment(f cmdutil.Factory, cmdOut, cmdErr io.Writer, cmd *cobra.Co
 	if err != nil {
 		return fmt.Errorf("failed to discover supported resources: %v", err)
 	}
+
 	generatorName := cmdutil.GetFlagString(cmd, "generator")
-	// fallback to the old generator if server does not support apps/v1beta1 deployments
-	if generatorName == cmdutil.DeploymentBasicAppsV1Beta1GeneratorName &&
-		!contains(resourcesList, appsv1beta1.SchemeGroupVersion.WithResource("deployments")) {
-		fmt.Fprintf(cmdErr, "WARNING: New deployments generator specified (%s), but apps/v1beta1.Deployments are not available, falling back to the old one (%s).\n",
-			cmdutil.DeploymentBasicAppsV1Beta1GeneratorName, cmdutil.DeploymentBasicV1Beta1GeneratorName)
-		generatorName = cmdutil.DeploymentBasicV1Beta1GeneratorName
-	}
-	var generator kubectl.StructuredGenerator
-	switch generatorName {
-	case cmdutil.DeploymentBasicAppsV1Beta1GeneratorName:
-		generator = &kubectl.DeploymentBasicAppsGeneratorV1{Name: name, Images: cmdutil.GetFlagStringSlice(cmd, "image")}
-	case cmdutil.DeploymentBasicV1Beta1GeneratorName:
-		generator = &kubectl.DeploymentBasicGeneratorV1{Name: name, Images: cmdutil.GetFlagStringSlice(cmd, "image")}
-	default:
+
+	// It is possible we have to modify the user-provided generator name if
+	// the server does not have support for the requested generator.
+	generatorName = fallbackGeneratorNameIfNecessary(generatorName, resourcesList, cmdErr)
+
+	imageNames := cmdutil.GetFlagStringSlice(cmd, "image")
+	generator, ok := generatorFromName(generatorName, imageNames, deploymentName)
+	if !ok {
 		return errUnsupportedGenerator(cmd, generatorName)
 	}
+
 	return RunCreateSubcommand(f, cmd, cmdOut, &CreateSubcommandOptions{
-		Name:                name,
+		Name:                deploymentName,
 		StructuredGenerator: generator,
 		DryRun:              cmdutil.GetDryRunFlag(cmd),
 		OutputFormat:        cmdutil.GetFlagString(cmd, "output"),

pkg/kubectl/cmd/create_deployment_test.go

@@ -27,9 +27,39 @@ import (
 	restclient "k8s.io/client-go/rest"
 	"k8s.io/client-go/rest/fake"
 	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/kubectl"
 	cmdtesting "k8s.io/kubernetes/pkg/kubectl/cmd/testing"
+	cmdutil "k8s.io/kubernetes/pkg/kubectl/cmd/util"
 )
 
+func Test_generatorFromName(t *testing.T) {
+	const (
+		nonsenseName   = "not-a-real-generator-name"
+		basicName      = cmdutil.DeploymentBasicV1Beta1GeneratorName
+		basicAppsName  = cmdutil.DeploymentBasicAppsV1Beta1GeneratorName
+		deploymentName = "deployment-name"
+	)
+	imageNames := []string{"image-1", "image-2"}
+
+	generator, ok := generatorFromName(nonsenseName, imageNames, deploymentName)
+	assert.Nil(t, generator)
+	assert.False(t, ok)
+
+	generator, ok = generatorFromName(basicName, imageNames, deploymentName)
+	assert.True(t, ok)
+	assert.Equal(t, &kubectl.DeploymentBasicGeneratorV1{
+		Name:   deploymentName,
+		Images: imageNames,
+	}, generator)
+
+	generator, ok = generatorFromName(basicAppsName, imageNames, deploymentName)
+	assert.True(t, ok)
+	assert.Equal(t, &kubectl.DeploymentBasicAppsGeneratorV1{
+		Name:   deploymentName,
+		Images: imageNames,
+	}, generator)
+}
+
 func TestCreateDeployment(t *testing.T) {
 	depName := "jonny-dep"
 	f, tf, _, ns := cmdtesting.NewAPIFactory()

pkg/kubectl/cmd/run.go

@@ -23,7 +23,6 @@ import (
 	"github.com/docker/distribution/reference"
 	"github.com/spf13/cobra"
 
-	appsv1beta1 "k8s.io/api/apps/v1beta1"
 	batchv1 "k8s.io/api/batch/v1"
 	batchv2alpha1 "k8s.io/api/batch/v2alpha1"
 	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
@@ -233,12 +232,7 @@ func RunRun(f cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer, cmd *c
 	}
 
 	// TODO: this should be removed alongside with extensions/v1beta1 depployments generator
-	if generatorName == cmdutil.DeploymentAppsV1Beta1GeneratorName &&
-		!contains(resourcesList, appsv1beta1.SchemeGroupVersion.WithResource("deployments")) {
-		fmt.Fprintf(cmdErr, "WARNING: New deployments generator specified (%s), but apps/v1beta1.Deployments are not available, falling back to the old one (%s).\n",
-			cmdutil.DeploymentAppsV1Beta1GeneratorName, cmdutil.DeploymentV1Beta1GeneratorName)
-		generatorName = cmdutil.DeploymentV1Beta1GeneratorName
-	}
+	generatorName = fallbackGeneratorNameIfNecessary(generatorName, resourcesList, cmdErr)
 
 	if generatorName == cmdutil.CronJobV2Alpha1GeneratorName &&
 		!contains(resourcesList, batchv2alpha1.SchemeGroupVersion.WithResource("cronjobs")) {