From 56930674c90d0fdd931954c952ba581c982d1a3d Mon Sep 17 00:00:00 2001
From: zhengjiajin <zhengjiajin@caicloud.io>
Date: Sun, 24 Sep 2017 22:23:13 +0800
Subject: [PATCH] bug(cli)fix kubectl config unset unexist map key will add
 this key, should tell user this key not exist

---
 pkg/kubectl/cmd/config/set.go        |  3 ++
 pkg/kubectl/cmd/config/unset.go      | 25 +++++++++-------
 pkg/kubectl/cmd/config/unset_test.go | 43 ++++++++++++++++++++++++----
 3 files changed, 55 insertions(+), 16 deletions(-)

diff --git a/pkg/kubectl/cmd/config/set.go b/pkg/kubectl/cmd/config/set.go
index c528f42761..03f97580a0 100644
--- a/pkg/kubectl/cmd/config/set.go
+++ b/pkg/kubectl/cmd/config/set.go
@@ -152,6 +152,9 @@ func modifyConfig(curr reflect.Value, steps *navigationSteps, propertyValue stri
 
 		needToSetNewMapValue := currMapValue.Kind() == reflect.Invalid
 		if needToSetNewMapValue {
+			if unset {
+				return fmt.Errorf("current map key `%v` is invalid", mapKey.Interface())
+			}
 			currMapValue = reflect.New(mapValueType.Elem()).Elem().Addr()
 			actualCurrValue.SetMapIndex(mapKey, currMapValue)
 		}
diff --git a/pkg/kubectl/cmd/config/unset.go b/pkg/kubectl/cmd/config/unset.go
index 2182e5596c..3d62038301 100644
--- a/pkg/kubectl/cmd/config/unset.go
+++ b/pkg/kubectl/cmd/config/unset.go
@@ -48,16 +48,16 @@ func NewCmdConfigUnset(out io.Writer, configAccess clientcmd.ConfigAccess) *cobr
 		Short: i18n.T("Unsets an individual value in a kubeconfig file"),
 		Long:  unset_long,
 		Run: func(cmd *cobra.Command, args []string) {
-			cmdutil.CheckErr(options.complete(cmd))
-			cmdutil.CheckErr(options.run())
-			fmt.Fprintf(out, "Property %q unset.\n", options.propertyName)
+			cmdutil.CheckErr(options.complete(cmd, args))
+			cmdutil.CheckErr(options.run(out))
+
 		},
 	}
 
 	return cmd
 }
 
-func (o unsetOptions) run() error {
+func (o unsetOptions) run(out io.Writer) error {
 	err := o.validate()
 	if err != nil {
 		return err
@@ -77,16 +77,21 @@ func (o unsetOptions) run() error {
 		return err
 	}
 
-	return clientcmd.ModifyConfig(o.configAccess, *config, false)
+	if err := clientcmd.ModifyConfig(o.configAccess, *config, false); err != nil {
+		return err
+	}
+	if _, err := fmt.Fprintf(out, "Property %q unset.\n", o.propertyName); err != nil {
+		return err
+	}
+	return nil
 }
 
-func (o *unsetOptions) complete(cmd *cobra.Command) error {
-	endingArgs := cmd.Flags().Args()
-	if len(endingArgs) != 1 {
-		return helpErrorf(cmd, "Unexpected args: %v", endingArgs)
+func (o *unsetOptions) complete(cmd *cobra.Command, args []string) error {
+	if len(args) != 1 {
+		return helpErrorf(cmd, "Unexpected args: %v", args)
 	}
 
-	o.propertyName = endingArgs[0]
+	o.propertyName = args[0]
 	return nil
 }
 
diff --git a/pkg/kubectl/cmd/config/unset_test.go b/pkg/kubectl/cmd/config/unset_test.go
index 1ca412056e..ea282a4d99 100644
--- a/pkg/kubectl/cmd/config/unset_test.go
+++ b/pkg/kubectl/cmd/config/unset_test.go
@@ -28,9 +28,10 @@ import (
 
 type unsetConfigTest struct {
 	description string
-	config      clientcmdapi.Config //initiate kubectl config
-	args        []string            //kubectl config unset args
-	expected    string              //expect out
+	config      clientcmdapi.Config
+	args        []string
+	expected    string
+	expectedErr string
 }
 
 func TestUnsetConfigString(t *testing.T) {
@@ -79,6 +80,31 @@ func TestUnsetConfigMap(t *testing.T) {
 	test.run(t)
 }
 
+func TestUnsetUnexistConfig(t *testing.T) {
+	conf := clientcmdapi.Config{
+		Kind:       "Config",
+		APIVersion: "v1",
+		Clusters: map[string]*clientcmdapi.Cluster{
+			"minikube":   {Server: "https://192.168.99.100:8443"},
+			"my-cluster": {Server: "https://192.168.0.1:3434"},
+		},
+		Contexts: map[string]*clientcmdapi.Context{
+			"minikube":   {AuthInfo: "minikube", Cluster: "minikube"},
+			"my-cluster": {AuthInfo: "mu-cluster", Cluster: "my-cluster"},
+		},
+		CurrentContext: "minikube",
+	}
+
+	test := unsetConfigTest{
+		description: "Testing for kubectl config unset a unexist map key",
+		config:      conf,
+		args:        []string{"contexts.foo.namespace"},
+		expectedErr: "current map key `foo` is invalid",
+	}
+	test.run(t)
+
+}
+
 func (test unsetConfigTest) run(t *testing.T) {
 	fakeKubeFile, err := ioutil.TempFile(os.TempDir(), "")
 	if err != nil {
@@ -94,14 +120,19 @@ func (test unsetConfigTest) run(t *testing.T) {
 	pathOptions.EnvVar = ""
 	buf := bytes.NewBuffer([]byte{})
 	cmd := NewCmdConfigUnset(buf, pathOptions)
-	cmd.SetArgs(test.args)
-	if err := cmd.Execute(); err != nil {
-		t.Fatalf("unexpected error executing command: %v,kubectl unset args: %v", err, test.args)
+	opts := &unsetOptions{configAccess: pathOptions}
+	err = opts.complete(cmd, test.args)
+	if err == nil {
+		err = opts.run(buf)
 	}
 	config, err := clientcmd.LoadFromFile(fakeKubeFile.Name())
 	if err != nil {
 		t.Fatalf("unexpected error loading kubeconfig file: %v", err)
 	}
+
+	if err != nil && err.Error() != test.expectedErr {
+		t.Fatalf("expected error:\n %v\nbut got error:\n%v", test.expectedErr, err)
+	}
 	if len(test.expected) != 0 {
 		if buf.String() != test.expected {
 			t.Errorf("Failed in :%q\n expected %v\n but got %v", test.description, test.expected, buf.String())