Merge pull request #832 from Liujingfang1/jsonpatch

improve error message in json patch transformer

Author: monopole

pkg/patch/transformer/factory.go

@@ -21,8 +21,6 @@ import (
 	"sigs.k8s.io/kustomize/pkg/ifc"
 	"sigs.k8s.io/kustomize/pkg/resid"
 
-	"github.com/evanphx/json-patch"
-	"github.com/ghodss/yaml"
 	"sigs.k8s.io/kustomize/pkg/gvk"
 	"sigs.k8s.io/kustomize/pkg/patch"
 	"sigs.k8s.io/kustomize/pkg/transformers"
@@ -77,19 +75,7 @@ func (f PatchJson6902Factory) makeOnePatchJson6902Transformer(p patch.Json6902)
 		return nil, err
 	}
 
-	if !isJsonFormat(rawOp) {
-		// if it isn't JSON, try to parse it as YAML
-		rawOp, err = yaml.YAMLToJSON(rawOp)
-		if err != nil {
-			return nil, err
-		}
-	}
-
-	decodedPatch, err := jsonpatch.DecodePatch(rawOp)
-	if err != nil {
-		return nil, err
-	}
-	return newPatchJson6902JSONTransformer(targetId, decodedPatch)
+	return newPatchJson6902JSONTransformer(targetId, rawOp)
 }
 
 func isJsonFormat(data []byte) bool {

pkg/patch/transformer/patchjson6902json.go

@@ -20,6 +20,8 @@ import (
 	"fmt"
 
 	"github.com/evanphx/json-patch"
+	"github.com/ghodss/yaml"
+	"github.com/pkg/errors"
 	"sigs.k8s.io/kustomize/pkg/resid"
 	"sigs.k8s.io/kustomize/pkg/resmap"
 	"sigs.k8s.io/kustomize/pkg/resource"
@@ -30,17 +32,31 @@ import (
 type patchJson6902JSONTransformer struct {
 	target resid.ResId
 	patch  jsonpatch.Patch
+	rawOp  []byte
 }
 
 var _ transformers.Transformer = &patchJson6902JSONTransformer{}
 
 // newPatchJson6902JSONTransformer constructs a PatchJson6902 transformer.
 func newPatchJson6902JSONTransformer(
-	id resid.ResId, p jsonpatch.Patch) (transformers.Transformer, error) {
-	if len(p) == 0 {
+	id resid.ResId, rawOp []byte) (transformers.Transformer, error) {
+	op := rawOp
+	var err error
+	if !isJsonFormat(op) {
+		// if it isn't JSON, try to parse it as YAML
+		op, err = yaml.YAMLToJSON(rawOp)
+		if err != nil {
+			return nil, err
+		}
+	}
+	decodedPatch, err := jsonpatch.DecodePatch(op)
+	if err != nil {
+		return nil, err
+	}
+	if len(decodedPatch) == 0 {
 		return transformers.NewNoOpTransformer(), nil
 	}
-	return &patchJson6902JSONTransformer{target: id, patch: p}, nil
+	return &patchJson6902JSONTransformer{target: id, patch: decodedPatch, rawOp: rawOp}, nil
 }
 
 // Transform apply the json patches on top of the base resources.
@@ -55,7 +71,7 @@ func (t *patchJson6902JSONTransformer) Transform(m resmap.ResMap) error {
 	}
 	modifiedObj, err := t.patch.Apply(rawObj)
 	if err != nil {
-		return err
+		return errors.Wrapf(err, "failed to apply json patch '%s'", string(t.rawOp))
 	}
 	err = obj.UnmarshalJSON(modifiedObj)
 	if err != nil {

pkg/patch/transformer/patchjson6902json_test.go

@@ -18,9 +18,9 @@ package transformer
 
 import (
 	"reflect"
+	"strings"
 	"testing"
 
-	"github.com/evanphx/json-patch"
 	"sigs.k8s.io/kustomize/k8sdeps/kunstruct"
 	"sigs.k8s.io/kustomize/pkg/gvk"
 	"sigs.k8s.io/kustomize/pkg/resid"
@@ -67,10 +67,7 @@ func TestJsonPatchJSONTransformer_Transform(t *testing.T) {
         {"op": "add", "path": "/spec/replica", "value": "3"},
         {"op": "add", "path": "/spec/template/spec/containers/0/command", "value": ["arg1", "arg2", "arg3"]}
 ]`)
-	patch, err := jsonpatch.DecodePatch(operations)
-	if err != nil {
-		t.Fatalf("unexpected error : %v", err)
-	}
+
 	expected := resmap.ResMap{
 		id: rf.FromMap(
 			map[string]interface{}{
@@ -104,7 +101,7 @@ func TestJsonPatchJSONTransformer_Transform(t *testing.T) {
 				},
 			}),
 	}
-	jpt, err := newPatchJson6902JSONTransformer(id, patch)
+	jpt, err := newPatchJson6902JSONTransformer(id, operations)
 	if err != nil {
 		t.Fatalf("unexpected error : %v", err)
 	}
@@ -117,3 +114,52 @@ func TestJsonPatchJSONTransformer_Transform(t *testing.T) {
 		t.Fatalf("actual doesn't match expected: %v", err)
 	}
 }
+
+func TestJsonPatchJSONTransformer_UnHappyTransform(t *testing.T) {
+	rf := resource.NewFactory(
+		kunstruct.NewKunstructuredFactoryImpl())
+	id := resid.NewResId(deploy, "deploy1")
+	base := resmap.ResMap{
+		id: rf.FromMap(
+			map[string]interface{}{
+				"apiVersion": "apps/v1",
+				"kind":       "Deployment",
+				"metadata": map[string]interface{}{
+					"name": "deploy1",
+				},
+				"spec": map[string]interface{}{
+					"template": map[string]interface{}{
+						"metadata": map[string]interface{}{
+							"labels": map[string]interface{}{
+								"old-label": "old-value",
+							},
+						},
+						"spec": map[string]interface{}{
+							"containers": []interface{}{
+								map[string]interface{}{
+									"name":  "nginx",
+									"image": "nginx",
+								},
+							},
+						},
+					},
+				},
+			}),
+	}
+
+	operations := []byte(`[
+        {"op": "add", "path": "/spec/template/spec/containers/0/command/", "value": ["arg1", "arg2", "arg3"]}
+]`)
+
+	jpt, err := newPatchJson6902JSONTransformer(id, operations)
+	if err != nil {
+		t.Fatalf("unexpected error : %v", err)
+	}
+	err = jpt.Transform(base)
+	if err == nil {
+		t.Fatalf("expected error didn't happen")
+	}
+	if !strings.HasPrefix(err.Error(), "failed to apply json patch") || !strings.Contains(err.Error(), string(operations)) {
+		t.Fatalf("expected error didn't happen, but got %v", err)
+	}
+}