diff --git a/pkg/kubectl/cmd/expose.go b/pkg/kubectl/cmd/expose.go index f44236cc53a..8c0f7e82913 100644 --- a/pkg/kubectl/cmd/expose.go +++ b/pkg/kubectl/cmd/expose.go @@ -85,7 +85,8 @@ Examples: inline := util.GetFlagString(cmd, "overrides") if len(inline) > 0 { - util.Merge(service, inline, "Service") + service, err = util.Merge(service, inline, "Service") + checkErr(err) } // TODO: extract this flag to a central location, when such a location exists. diff --git a/pkg/kubectl/cmd/run.go b/pkg/kubectl/cmd/run.go index 09219db869d..974471b802e 100644 --- a/pkg/kubectl/cmd/run.go +++ b/pkg/kubectl/cmd/run.go @@ -74,7 +74,8 @@ Examples: inline := util.GetFlagString(cmd, "overrides") if len(inline) > 0 { - util.Merge(controller, inline, "ReplicationController") + controller, err = util.Merge(controller, inline, "ReplicationController") + checkErr(err) } // TODO: extract this flag to a central location, when such a location exists. diff --git a/pkg/kubectl/cmd/update.go b/pkg/kubectl/cmd/update.go index 86a19b051df..5b43e5260ef 100644 --- a/pkg/kubectl/cmd/update.go +++ b/pkg/kubectl/cmd/update.go @@ -116,12 +116,13 @@ func updateWithPatch(cmd *cobra.Command, args []string, f *Factory, patch string obj, err := helper.Get(namespace, name) checkErr(err) - cmdutil.Merge(obj, patch, mapping.Kind) - - data, err := helper.Codec.Encode(obj) + patchedObj, err := cmdutil.Merge(obj, patch, mapping.Kind) checkErr(err) - obj, err = helper.Update(namespace, name, true, data) + data, err := helper.Codec.Encode(patchedObj) + checkErr(err) + + _, err = helper.Update(namespace, name, true, data) checkErr(err) return name } diff --git a/pkg/kubectl/cmd/util/helpers.go b/pkg/kubectl/cmd/util/helpers.go index ca5d9d2b5ad..a2c2b485497 100644 --- a/pkg/kubectl/cmd/util/helpers.go +++ b/pkg/kubectl/cmd/util/helpers.go @@ -29,8 +29,8 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/api" "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" + "github.com/GoogleCloudPlatform/kubernetes/pkg/util/merge" "github.com/golang/glog" - "github.com/imdario/mergo" "github.com/spf13/cobra" ) @@ -188,36 +188,40 @@ func ReadConfigDataFromLocation(location string) ([]byte, error) { } } -func Merge(dst runtime.Object, fragment, kind string) error { +func Merge(dst runtime.Object, fragment, kind string) (runtime.Object, error) { // Ok, this is a little hairy, we'd rather not force the user to specify a kind for their JSON // So we pull it into a map, add the Kind field, and then reserialize. // We also pull the apiVersion for proper parsing var intermediate interface{} if err := json.Unmarshal([]byte(fragment), &intermediate); err != nil { - return err + return nil, err } dataMap, ok := intermediate.(map[string]interface{}) if !ok { - return fmt.Errorf("Expected a map, found something else: %s", fragment) + return nil, fmt.Errorf("Expected a map, found something else: %s", fragment) } version, found := dataMap["apiVersion"] if !found { - return fmt.Errorf("Inline JSON requires an apiVersion field") + return nil, fmt.Errorf("Inline JSON requires an apiVersion field") } versionString, ok := version.(string) if !ok { - return fmt.Errorf("apiVersion must be a string") + return nil, fmt.Errorf("apiVersion must be a string") } - codec := runtime.CodecFor(api.Scheme, versionString) - dataMap["kind"] = kind - data, err := json.Marshal(intermediate) + codec := runtime.CodecFor(api.Scheme, versionString) + // encode dst into versioned json and apply fragment directly too it + target, err := codec.Encode(dst) if err != nil { - return err + return nil, err } - src, err := codec.Decode(data) + patched, err := merge.MergeJSON(target, []byte(fragment)) if err != nil { - return err + return nil, err } - return mergo.Merge(dst, src) + out, err := codec.Decode(patched) + if err != nil { + return nil, err + } + return out, nil } diff --git a/pkg/kubectl/cmd/util/helpers_test.go b/pkg/kubectl/cmd/util/helpers_test.go index 32673373c74..751f4712dc1 100644 --- a/pkg/kubectl/cmd/util/helpers_test.go +++ b/pkg/kubectl/cmd/util/helpers_test.go @@ -30,14 +30,16 @@ func TestMerge(t *testing.T) { fragment string expected runtime.Object expectErr bool + kind string }{ { + kind: "Pod", obj: &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "foo", }, }, - fragment: "{ \"apiVersion\": \"v1beta1\" }", + fragment: `{ "apiVersion": "v1beta1" }`, expected: &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "foo", @@ -51,12 +53,13 @@ func TestMerge(t *testing.T) { }, }, { + kind: "Pod", obj: &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "foo", }, }, - fragment: "{ \"apiVersion\": \"v1beta1\", \"id\": \"baz\", \"desiredState\": { \"host\": \"bar\" } }", + fragment: `{ "apiVersion": "v1beta1", "id": "baz", "desiredState": { "host": "bar" } }`, expected: &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "baz", @@ -71,12 +74,13 @@ func TestMerge(t *testing.T) { }, }, { + kind: "Pod", obj: &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "foo", }, }, - fragment: "{ \"apiVersion\": \"v1beta3\", \"spec\": { \"volumes\": [ {\"name\": \"v1\"}, {\"name\": \"v2\"} ] } }", + fragment: `{ "apiVersion": "v1beta3", "spec": { "volumes": [ {"name": "v1"}, {"name": "v2"} ] } }`, expected: &api.Pod{ ObjectMeta: api.ObjectMeta{ Name: "foo", @@ -100,24 +104,81 @@ func TestMerge(t *testing.T) { }, }, { + kind: "Pod", obj: &api.Pod{}, fragment: "invalid json", expected: &api.Pod{}, expectErr: true, }, + { + kind: "Pod", + obj: &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: "foo", + }, + }, + fragment: `{ "apiVersion": "v1beta1", "id": null}`, + expected: &api.Pod{ + ObjectMeta: api.ObjectMeta{ + Name: "", + }, + Spec: api.PodSpec{ + RestartPolicy: api.RestartPolicy{ + Always: &api.RestartPolicyAlways{}, + }, + DNSPolicy: api.DNSClusterFirst, + }, + }, + }, + { + kind: "Service", + obj: &api.Service{ + Spec: api.ServiceSpec{ + Port: 10, + }, + }, + fragment: `{ "apiVersion": "v1beta1", "port": 0 }`, + expected: &api.Service{ + Spec: api.ServiceSpec{ + Port: 0, + Protocol: "TCP", + SessionAffinity: "None", + }, + }, + }, + { + kind: "Service", + obj: &api.Service{ + Spec: api.ServiceSpec{ + Selector: map[string]string{ + "version": "v1", + }, + }, + }, + fragment: `{ "apiVersion": "v1beta1", "selector": { "version": "v2" } }`, + expected: &api.Service{ + Spec: api.ServiceSpec{ + Protocol: "TCP", + SessionAffinity: "None", + Selector: map[string]string{ + "version": "v2", + }, + }, + }, + }, } for i, test := range tests { - err := Merge(test.obj, test.fragment, "Pod") + out, err := Merge(test.obj, test.fragment, test.kind) if !test.expectErr { if err != nil { - t.Errorf("unexpected error: %v", err) - } else if !reflect.DeepEqual(test.obj, test.expected) { - t.Errorf("\n\ntestcase[%d]\nexpected:\n%v\nsaw:\n%v", i, test.expected, test.obj) + t.Errorf("testcase[%d], unexpected error: %v", i, err) + } else if !reflect.DeepEqual(out, test.expected) { + t.Errorf("\n\ntestcase[%d]\nexpected:\n%+v\nsaw:\n%+v", i, test.expected, out) } } if test.expectErr && err == nil { - t.Errorf("unexpected non-error") + t.Errorf("testcase[%d], unexpected non-error", i) } }