Merge pull request #49223 from alexandercampbell/kubectl-impl-only-structuredgenerator

Automatic merge from submit-queue

kubectl: deploy generators don't need to impl Generator iface

The `kubectl create deployment` generators do not need to implement the Generator interface, since they are only used as implementations of the StructuredGenerator interface. I was able to delete some tests of their Generator methods as part of this change.

### Considerations for code reviewers

1. Every other StructuredGenerator implementation implements the Generator interface in additional. My change makes the "create deployment" generators a little unusual. I've added a docstring to this effect in `util/factory_client_access.go`.
2. This significantly reduces the maintenance / testing burden for future updates to `kubectl create deployment`.

**Release note**:

```release-note
NONE
```

pkg/kubectl/BUILD

@@ -14,7 +14,6 @@ go_test(
         "cluster_test.go",
         "configmap_test.go",
         "delete_test.go",
-        "deployment_test.go",
         "env_file_test.go",
         "generate_test.go",
         "kubectl_test.go",

pkg/kubectl/cmd/util/factory_client_access.go

@@ -510,10 +510,12 @@ func DefaultGenerators(cmdName string) map[string]kubectl.Generator {
 			ServiceLoadBalancerGeneratorV1Name: kubectl.ServiceLoadBalancerGeneratorV1{},
 		}
 	case "deployment":
-		generator = map[string]kubectl.Generator{
+		// Create Deployment has only StructuredGenerators and no
-			DeploymentBasicV1Beta1GeneratorName:     kubectl.DeploymentBasicGeneratorV1{},
+		// param-based Generators.
-			DeploymentBasicAppsV1Beta1GeneratorName: kubectl.DeploymentBasicAppsGeneratorV1{},
+		// The StructuredGenerators are as follows (as of 2017-07-17):
-		}
+		// DeploymentBasicV1Beta1GeneratorName -> kubectl.DeploymentBasicGeneratorV1
+		// DeploymentBasicAppsV1Beta1GeneratorName -> kubectl.DeploymentBasicAppsGeneratorV1
+		generator = map[string]kubectl.Generator{}
 	case "run":
 		generator = map[string]kubectl.Generator{
 			RunV1GeneratorName:                 kubectl.BasicReplicationController{},

pkg/kubectl/deployment.go

@@ -36,16 +36,6 @@ type BaseDeploymentGenerator struct {
 	Images []string
 }
 
-// ParamNames: return the parameters expected by the BaseDeploymentGenerator.
-// This method is here to aid in validation. When given a Generator, you can
-// learn what it expects by calling this method.
-func (BaseDeploymentGenerator) ParamNames() []GeneratorParam {
-	return []GeneratorParam{
-		{"name", true},
-		{"image", true},
-	}
-}
-
 // validate: check if the caller has forgotten to set one of our fields.
 func (b BaseDeploymentGenerator) validate() error {
 	if len(b.Name) == 0 {
@@ -57,29 +47,6 @@ func (b BaseDeploymentGenerator) validate() error {
 	return nil
 }
 
-// baseDeploymentGeneratorFromParams: return a new BaseDeploymentGenerator with
-// the fields set from params. The returned BaseDeploymentGenerator should have
-// all required fields set and will pass validate() with no errors.
-func baseDeploymentGeneratorFromParams(params map[string]interface{}) (*BaseDeploymentGenerator, error) {
-	paramNames := (BaseDeploymentGenerator{}).ParamNames()
-	err := ValidateParams(paramNames, params)
-	if err != nil {
-		return nil, err
-	}
-	name, isString := params["name"].(string)
-	if !isString {
-		return nil, fmt.Errorf("expected string, saw %v for 'name'", name)
-	}
-	imageStrings, isArray := params["image"].([]string)
-	if !isArray {
-		return nil, fmt.Errorf("expected []string, found :%v", imageStrings)
-	}
-	return &BaseDeploymentGenerator{
-		Name:   name,
-		Images: imageStrings,
-	}, nil
-}
-
 // structuredGenerate: determine the fields of a deployment. The struct that
 // embeds BaseDeploymentGenerator should assemble these pieces into a
 // runtime.Object.
@@ -127,14 +94,6 @@ type DeploymentBasicGeneratorV1 struct {
 // Ensure it supports the generator pattern that uses parameters specified during construction
 var _ StructuredGenerator = &DeploymentBasicGeneratorV1{}
 
-func (s DeploymentBasicGeneratorV1) Generate(params map[string]interface{}) (runtime.Object, error) {
-	base, err := baseDeploymentGeneratorFromParams(params)
-	if err != nil {
-		return nil, err
-	}
-	return (&DeploymentBasicGeneratorV1{*base}).StructuredGenerate()
-}
-
 // StructuredGenerate outputs a deployment object using the configured fields
 func (s *DeploymentBasicGeneratorV1) StructuredGenerate() (runtime.Object, error) {
 	podSpec, labels, selector, err := s.structuredGenerate()
@@ -165,14 +124,6 @@ type DeploymentBasicAppsGeneratorV1 struct {
 // Ensure it supports the generator pattern that uses parameters specified during construction
 var _ StructuredGenerator = &DeploymentBasicAppsGeneratorV1{}
 
-func (s DeploymentBasicAppsGeneratorV1) Generate(params map[string]interface{}) (runtime.Object, error) {
-	base, err := baseDeploymentGeneratorFromParams(params)
-	if err != nil {
-		return nil, err
-	}
-	return (&DeploymentBasicAppsGeneratorV1{*base}).StructuredGenerate()
-}
-
 // StructuredGenerate outputs a deployment object using the configured fields
 func (s *DeploymentBasicAppsGeneratorV1) StructuredGenerate() (runtime.Object, error) {
 	podSpec, labels, selector, err := s.structuredGenerate()

pkg/kubectl/deployment_test.go

@@ -1,247 +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.
-*/
-
-package kubectl
-
-import (
-	"reflect"
-	"testing"
-
-	appsv1beta1 "k8s.io/api/apps/v1beta1"
-	"k8s.io/api/core/v1"
-	extensionsv1beta1 "k8s.io/api/extensions/v1beta1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-)
-
-func TestDeploymentGenerate(t *testing.T) {
-	one := int32(1)
-	tests := []struct {
-		params    map[string]interface{}
-		expected  *extensionsv1beta1.Deployment
-		expectErr bool
-	}{
-		{
-			params: map[string]interface{}{
-				"name":  "foo",
-				"image": []string{"abc/app:v4"},
-			},
-			expected: &extensionsv1beta1.Deployment{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:   "foo",
-					Labels: map[string]string{"app": "foo"},
-				},
-				Spec: extensionsv1beta1.DeploymentSpec{
-					Replicas: &one,
-					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "foo"}},
-					Template: v1.PodTemplateSpec{
-						ObjectMeta: metav1.ObjectMeta{
-							Labels: map[string]string{"app": "foo"},
-						},
-						Spec: v1.PodSpec{
-							Containers: []v1.Container{{Name: "app", Image: "abc/app:v4"}},
-						},
-					},
-				},
-			},
-			expectErr: false,
-		},
-		{
-			params: map[string]interface{}{
-				"name":  "foo",
-				"image": []string{"abc/app:v4", "zyx/ape"},
-			},
-			expected: &extensionsv1beta1.Deployment{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:   "foo",
-					Labels: map[string]string{"app": "foo"},
-				},
-				Spec: extensionsv1beta1.DeploymentSpec{
-					Replicas: &one,
-					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "foo"}},
-					Template: v1.PodTemplateSpec{
-						ObjectMeta: metav1.ObjectMeta{
-							Labels: map[string]string{"app": "foo"},
-						},
-						Spec: v1.PodSpec{
-							Containers: []v1.Container{{Name: "app", Image: "abc/app:v4"},
-								{Name: "ape", Image: "zyx/ape"}},
-						},
-					},
-				},
-			},
-			expectErr: false,
-		},
-		{
-			params:    map[string]interface{}{},
-			expectErr: true,
-		},
-		{
-			params: map[string]interface{}{
-				"name": 1,
-			},
-			expectErr: true,
-		},
-		{
-			params: map[string]interface{}{
-				"name": nil,
-			},
-			expectErr: true,
-		},
-		{
-			params: map[string]interface{}{
-				"name":  "foo",
-				"image": []string{},
-			},
-			expectErr: true,
-		},
-		{
-			params: map[string]interface{}{
-				"NAME": "some_value",
-			},
-			expectErr: true,
-		},
-	}
-	generator := DeploymentBasicGeneratorV1{}
-	for index, test := range tests {
-		t.Logf("running scenario %d", index)
-		obj, err := generator.Generate(test.params)
-		switch {
-		case test.expectErr && err != nil:
-			continue // loop, since there's no output to check
-		case test.expectErr && err == nil:
-			t.Errorf("expected error and didn't get one")
-			continue // loop, no expected output object
-		case !test.expectErr && err != nil:
-			t.Errorf("unexpected error %v", err)
-			continue // loop, no output object
-		case !test.expectErr && err == nil:
-			// do nothing and drop through
-		}
-		if !reflect.DeepEqual(obj.(*extensionsv1beta1.Deployment), test.expected) {
-			t.Errorf("expected:\n%#v\nsaw:\n%#v", test.expected, obj.(*extensionsv1beta1.Deployment))
-		}
-	}
-}
-
-func TestAppsDeploymentGenerate(t *testing.T) {
-	one := int32(1)
-	tests := []struct {
-		params    map[string]interface{}
-		expected  *appsv1beta1.Deployment
-		expectErr bool
-	}{
-		{
-			params: map[string]interface{}{
-				"name":  "foo",
-				"image": []string{"abc/app:v4"},
-			},
-			expected: &appsv1beta1.Deployment{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:   "foo",
-					Labels: map[string]string{"app": "foo"},
-				},
-				Spec: appsv1beta1.DeploymentSpec{
-					Replicas: &one,
-					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "foo"}},
-					Template: v1.PodTemplateSpec{
-						ObjectMeta: metav1.ObjectMeta{
-							Labels: map[string]string{"app": "foo"},
-						},
-						Spec: v1.PodSpec{
-							Containers: []v1.Container{{Name: "app", Image: "abc/app:v4"}},
-						},
-					},
-				},
-			},
-			expectErr: false,
-		},
-		{
-			params: map[string]interface{}{
-				"name":  "foo",
-				"image": []string{"abc/app:v4", "zyx/ape"},
-			},
-			expected: &appsv1beta1.Deployment{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:   "foo",
-					Labels: map[string]string{"app": "foo"},
-				},
-				Spec: appsv1beta1.DeploymentSpec{
-					Replicas: &one,
-					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"app": "foo"}},
-					Template: v1.PodTemplateSpec{
-						ObjectMeta: metav1.ObjectMeta{
-							Labels: map[string]string{"app": "foo"},
-						},
-						Spec: v1.PodSpec{
-							Containers: []v1.Container{{Name: "app", Image: "abc/app:v4"},
-								{Name: "ape", Image: "zyx/ape"}},
-						},
-					},
-				},
-			},
-			expectErr: false,
-		},
-		{
-			params:    map[string]interface{}{},
-			expectErr: true,
-		},
-		{
-			params: map[string]interface{}{
-				"name": 1,
-			},
-			expectErr: true,
-		},
-		{
-			params: map[string]interface{}{
-				"name": nil,
-			},
-			expectErr: true,
-		},
-		{
-			params: map[string]interface{}{
-				"name":  "foo",
-				"image": []string{},
-			},
-			expectErr: true,
-		},
-		{
-			params: map[string]interface{}{
-				"NAME": "some_value",
-			},
-			expectErr: true,
-		},
-	}
-	generator := DeploymentBasicAppsGeneratorV1{}
-	for index, test := range tests {
-		t.Logf("running scenario %d", index)
-		obj, err := generator.Generate(test.params)
-		switch {
-		case test.expectErr && err != nil:
-			continue // loop, since there's no output to check
-		case test.expectErr && err == nil:
-			t.Errorf("expected error and didn't get one")
-			continue // loop, no expected output object
-		case !test.expectErr && err != nil:
-			t.Errorf("unexpected error %v", err)
-			continue // loop, no output object
-		case !test.expectErr && err == nil:
-			// do nothing and drop through
-		}
-		if !reflect.DeepEqual(obj.(*appsv1beta1.Deployment), test.expected) {
-			t.Errorf("expected:\n%#v\nsaw:\n%#v", test.expected, obj.(*appsv1beta1.Deployment))
-		}
-	}
-}