From bc1a1d7b300e2996406be800dc7070071760d371 Mon Sep 17 00:00:00 2001
From: Fabiano Franz <ffranz@redhat.com>
Date: Thu, 27 Oct 2016 20:05:26 -0200
Subject: [PATCH] Better kubectl run validations

---
 pkg/kubectl/cmd/run.go          | 28 +++++-----
 pkg/kubectl/cmd/run_test.go     | 96 +++++++++++++++++++++++++++++++++
 pkg/kubectl/cmd/testing/fake.go | 11 +++-
 3 files changed, 121 insertions(+), 14 deletions(-)

diff --git a/pkg/kubectl/cmd/run.go b/pkg/kubectl/cmd/run.go
index bd937656ec4..b6be2cf16da 100644
--- a/pkg/kubectl/cmd/run.go
+++ b/pkg/kubectl/cmd/run.go
@@ -174,6 +174,22 @@ func Run(f cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer, cmd *cobr
 		return cmdutil.UsageError(cmd, fmt.Sprintf("--restart=%s requires that --replicas=1, found %d", restartPolicy, replicas))
 	}
 
+	attachFlag := cmd.Flags().Lookup("attach")
+	attach := cmdutil.GetFlagBool(cmd, "attach")
+
+	if !attachFlag.Changed && interactive {
+		attach = true
+	}
+
+	remove := cmdutil.GetFlagBool(cmd, "rm")
+	if !attach && remove {
+		return cmdutil.UsageError(cmd, "--rm should only be used for attached containers")
+	}
+
+	if attach && cmdutil.GetDryRunFlag(cmd) {
+		return cmdutil.UsageError(cmd, "--dry-run can't be used with attached containers options (--attach, --stdin, or --tty)")
+	}
+
 	if err := verifyImagePullPolicy(cmd); err != nil {
 		return err
 	}
@@ -241,18 +257,6 @@ func Run(f cmdutil.Factory, cmdIn io.Reader, cmdOut, cmdErr io.Writer, cmd *cobr
 		}
 	}
 
-	attachFlag := cmd.Flags().Lookup("attach")
-	attach := cmdutil.GetFlagBool(cmd, "attach")
-
-	if !attachFlag.Changed && interactive {
-		attach = true
-	}
-
-	remove := cmdutil.GetFlagBool(cmd, "rm")
-	if !attach && remove {
-		return cmdutil.UsageError(cmd, "--rm should only be used for attached containers")
-	}
-
 	if attach {
 		quiet := cmdutil.GetFlagBool(cmd, "quiet")
 		opts := &AttachOptions{
diff --git a/pkg/kubectl/cmd/run_test.go b/pkg/kubectl/cmd/run_test.go
index 865d9efb83f..c34b894a037 100644
--- a/pkg/kubectl/cmd/run_test.go
+++ b/pkg/kubectl/cmd/run_test.go
@@ -23,10 +23,12 @@ import (
 	"net/http"
 	"os"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/spf13/cobra"
 	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/apimachinery/registered"
 	"k8s.io/kubernetes/pkg/client/restclient"
 	"k8s.io/kubernetes/pkg/client/unversioned/fake"
@@ -332,3 +334,97 @@ func TestGenerateService(t *testing.T) {
 		}
 	}
 }
+
+func TestRunValidations(t *testing.T) {
+	tests := []struct {
+		args        []string
+		flags       map[string]string
+		expectedErr string
+	}{
+		{
+			expectedErr: "NAME is required",
+		},
+		{
+			args:        []string{"test"},
+			expectedErr: "Invalid image name",
+		},
+		{
+			args: []string{"test"},
+			flags: map[string]string{
+				"image":    "busybox",
+				"stdin":    "true",
+				"replicas": "2",
+			},
+			expectedErr: "stdin requires that replicas is 1",
+		},
+		{
+			args: []string{"test"},
+			flags: map[string]string{
+				"image": "busybox",
+				"rm":    "true",
+			},
+			expectedErr: "rm should only be used for attached containers",
+		},
+		{
+			args: []string{"test"},
+			flags: map[string]string{
+				"image":   "busybox",
+				"attach":  "true",
+				"dry-run": "true",
+			},
+			expectedErr: "can't be used with attached containers options",
+		},
+		{
+			args: []string{"test"},
+			flags: map[string]string{
+				"image":   "busybox",
+				"stdin":   "true",
+				"dry-run": "true",
+			},
+			expectedErr: "can't be used with attached containers options",
+		},
+		{
+			args: []string{"test"},
+			flags: map[string]string{
+				"image":   "busybox",
+				"tty":     "true",
+				"stdin":   "true",
+				"dry-run": "true",
+			},
+			expectedErr: "can't be used with attached containers options",
+		},
+		{
+			args: []string{"test"},
+			flags: map[string]string{
+				"image": "busybox",
+				"tty":   "true",
+			},
+			expectedErr: "stdin is required for containers with -t/--tty",
+		},
+	}
+	for _, test := range tests {
+		f, tf, codec, ns := cmdtesting.NewTestFactory()
+		tf.Printer = &testPrinter{}
+		tf.Client = &fake.RESTClient{
+			NegotiatedSerializer: ns,
+			Resp:                 &http.Response{StatusCode: 200, Header: defaultHeader(), Body: objBody(codec, cmdtesting.NewInternalType("", "", ""))},
+		}
+		tf.Namespace = "test"
+		tf.ClientConfig = &restclient.Config{ContentConfig: restclient.ContentConfig{GroupVersion: &unversioned.GroupVersion{Version: "v1"}}}
+		inBuf := bytes.NewReader([]byte{})
+		outBuf := bytes.NewBuffer([]byte{})
+		errBuf := bytes.NewBuffer([]byte{})
+
+		cmd := NewCmdRun(f, inBuf, outBuf, errBuf)
+		for flagName, flagValue := range test.flags {
+			cmd.Flags().Set(flagName, flagValue)
+		}
+		err := Run(f, inBuf, outBuf, errBuf, cmd, test.args, cmd.ArgsLenAtDash())
+		if err != nil && len(test.expectedErr) > 0 {
+			if !strings.Contains(err.Error(), test.expectedErr) {
+				t.Errorf("unexpected error: %v", err)
+			}
+		}
+	}
+
+}
diff --git a/pkg/kubectl/cmd/testing/fake.go b/pkg/kubectl/cmd/testing/fake.go
index 711ae9516d7..cf7a971ba8c 100644
--- a/pkg/kubectl/cmd/testing/fake.go
+++ b/pkg/kubectl/cmd/testing/fake.go
@@ -283,8 +283,15 @@ func (f *FakeFactory) DefaultNamespace() (string, bool, error) {
 	return f.tf.Namespace, false, f.tf.Err
 }
 
-func (f *FakeFactory) Generators(string) map[string]kubectl.Generator {
-	return nil
+func (f *FakeFactory) Generators(cmdName string) map[string]kubectl.Generator {
+	var generator map[string]kubectl.Generator
+	switch cmdName {
+	case "run":
+		generator = map[string]kubectl.Generator{
+			cmdutil.DeploymentV1Beta1GeneratorName: kubectl.DeploymentV1Beta1{},
+		}
+	}
+	return generator
 }
 
 func (f *FakeFactory) CanBeExposed(unversioned.GroupKind) error {