Merge pull request #310 from Liujingfang1/patchtransformer

Update patch factory and add multi transformer with checking conflicts

pkg/patch/transformer/factory.go

@@ -20,7 +20,6 @@ import (
 	"fmt"
 
 	"github.com/evanphx/json-patch"
-	"github.com/krishicks/yaml-patch"
 	"github.com/kubernetes-sigs/kustomize/pkg/loader"
 	"github.com/kubernetes-sigs/kustomize/pkg/patch"
 	"github.com/kubernetes-sigs/kustomize/pkg/resource"
@@ -28,16 +27,32 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
-// PatchJson6902Factory makes PatchJson6902 transformers.
+// PatchJson6902Factory makes PatchJson6902 transformers
 type PatchJson6902Factory struct {
-	targetId       resource.ResId
-	operationsYAML yamlpatch.Patch
-	operationsJSON jsonpatch.Patch
+	loader loader.Loader
 }
 
 // NewPatchJson6902Factory returns a new PatchJson6902Factory.
-func NewPatchJson6902Factory(l loader.Loader, p patch.PatchJson6902) (*PatchJson6902Factory, error) {
+func NewPatchJson6902Factory(l loader.Loader) PatchJson6902Factory {
+	return PatchJson6902Factory{loader: l}
+}
 
+// MakePatchJson6902Transformer returns a transformer for applying Json6902 patch
+func (f PatchJson6902Factory) MakePatchJson6902Transformer(patches []patch.PatchJson6902) (transformers.Transformer, error) {
+	var ts []transformers.Transformer
+	for _, p := range patches {
+		t, err := f.makeOnePatchJson6902Transformer(p)
+		if err != nil {
+			return nil, err
+		}
+		if t != nil {
+			ts = append(ts, t)
+		}
+	}
+	return transformers.NewMultiTransformerWithConflictCheck(ts), nil
+}
+
+func (f PatchJson6902Factory) makeOnePatchJson6902Transformer(p patch.PatchJson6902) (transformers.Transformer, error) {
 	if p.Target == nil {
 		return nil, fmt.Errorf("must specify the target field in patchesJson6902")
 	}
@@ -57,26 +72,19 @@ func NewPatchJson6902Factory(l loader.Loader, p patch.PatchJson6902) (*PatchJson
 	)
 
 	if p.JsonPatch != nil {
-		return &PatchJson6902Factory{targetId: targetId, operationsYAML: p.JsonPatch}, nil
+		return newPatchJson6902YAMLTransformer(targetId, p.JsonPatch)
 	}
 	if p.Path != "" {
-		rawOp, err := l.Load(p.Path)
+		rawOp, err := f.loader.Load(p.Path)
 		if err != nil {
 			return nil, err
 		}
-		patch, err := jsonpatch.DecodePatch(rawOp)
+		decodedPatch, err := jsonpatch.DecodePatch(rawOp)
 		if err != nil {
 			return nil, err
 		}
-		return &PatchJson6902Factory{targetId: targetId, operationsJSON: patch}, nil
+		return newPatchJson6902JSONTransformer(targetId, decodedPatch)
+
 	}
 	return nil, nil
 }
-
-// MakePatchJson6902Transformer returns a transformer for applying Json6902 patch
-func (f *PatchJson6902Factory) MakePatchJson6902Transformer() (transformers.Transformer, error) {
-	if f.operationsJSON != nil {
-		return newPatchJson6902JSONTransformer(f.targetId, f.operationsJSON)
-	}
-	return newPatchJson6902YAMLTransformer(f.targetId, f.operationsYAML)
-}

pkg/patch/transformer/factory_test.go

@@ -17,12 +17,16 @@ limitations under the License.
 package transformer
 
 import (
+	"reflect"
 	"strings"
 	"testing"
 
 	"github.com/kubernetes-sigs/kustomize/pkg/internal/loadertest"
 	"github.com/kubernetes-sigs/kustomize/pkg/patch"
+	"github.com/kubernetes-sigs/kustomize/pkg/resmap"
+	"github.com/kubernetes-sigs/kustomize/pkg/resource"
 	"gopkg.in/yaml.v2"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 )
 
 func TestNewPatchJson6902FactoryNull(t *testing.T) {
@@ -31,18 +35,19 @@ func TestNewPatchJson6902FactoryNull(t *testing.T) {
 			Name: "some-name",
 		},
 	}
-	f, err := NewPatchJson6902Factory(nil, p)
+	f := NewPatchJson6902Factory(nil)
+	tr, err := f.makeOnePatchJson6902Transformer(p)
 	if err != nil {
 		t.Fatalf("unexpected error : %v", err)
 	}
-	if f != nil {
+	if tr != nil {
 		t.Fatal("a nil should be returned")
 	}
 }
 
 func TestNewPatchJson6902FactoryNoTarget(t *testing.T) {
 	p := patch.PatchJson6902{}
-	_, err := NewPatchJson6902Factory(nil, p)
+	_, err := NewPatchJson6902Factory(nil).makeOnePatchJson6902Transformer(p)
 	if err == nil {
 		t.Fatal("expected error")
 	}
@@ -70,7 +75,8 @@ path: /some/dir/some/file
 	if err != nil {
 		t.Fatalf("expected error %v", err)
 	}
-	_, err = NewPatchJson6902Factory(nil, p)
+	f := NewPatchJson6902Factory(nil)
+	_, err = f.makeOnePatchJson6902Transformer(p)
 	if err == nil {
 		t.Fatal("expected error")
 	}
@@ -103,17 +109,13 @@ path: /testpath/patch.json
 		t.Fatal("expected error")
 	}
 
-	f, err := NewPatchJson6902Factory(ldr, p)
+	f := NewPatchJson6902Factory(ldr)
+	tr, err := f.makeOnePatchJson6902Transformer(p)
 	if err != nil {
 		t.Fatalf("unexpected error : %v", err)
 	}
-	if f == nil {
-		t.Fatalf("the returned factory shouldn't be nil ")
-	}
-
-	_, err = f.MakePatchJson6902Transformer()
-	if err != nil {
-		t.Fatalf("unexpected error : %v", err)
+	if tr == nil {
+		t.Fatal("the returned transformer should not be nil")
 	}
 }
 
@@ -139,17 +141,197 @@ jsonPatch:
 		t.Fatalf("unexpected error : %v", err)
 	}
 
-	f, err := NewPatchJson6902Factory(nil, p)
+	f := NewPatchJson6902Factory(nil)
+	tr, err := f.makeOnePatchJson6902Transformer(p)
 	if err != nil {
 		t.Fatalf("unexpected error : %v", err)
 	}
-	if f == nil {
-		t.Fatalf("the returned factory shouldn't be nil ")
+	if tr == nil {
+		t.Fatal("the returned transformer should not be nil")
+	}
+}
+
+func TestNewPatchJson6902FactoryMulti(t *testing.T) {
+	ldr := loadertest.NewFakeLoader("/testpath")
+	operations := []byte(`[
+        {"op": "replace", "path": "/spec/template/spec/containers/0/name", "value": "my-nginx"},
+        {"op": "add", "path": "/spec/replica", "value": "3"}
+]`)
+	err := ldr.AddFile("/testpath/patch.json", operations)
+	if err != nil {
+		t.Fatalf("Failed to setup fake ldr.")
+	}
+
+	jsonPatches := []byte(`
+- target:
+    kind: foo
+    name: some-name
+  path: /testpath/patch.json
+
+- target:
+    kind: foo
+    name: some-name
+  jsonPatch:
+  - op: add
+    path: /spec/template/spec/containers/0/command
+    value: ["arg1", "arg2", "arg3"]
+`)
+	var p []patch.PatchJson6902
+	err = yaml.Unmarshal(jsonPatches, &p)
+	if err != nil {
+		t.Fatalf("unexpected error : %v", err)
+	}
+
+	f := NewPatchJson6902Factory(ldr)
+	tr, err := f.MakePatchJson6902Transformer(p)
+	if err != nil {
+		t.Fatalf("unexpected error : %v", err)
+	}
+	if tr == nil {
+		t.Fatal("the returned transformer should not be nil")
+	}
+
+	id := resource.NewResId(schema.GroupVersionKind{Kind: "foo"}, "some-name")
+	base := resmap.ResMap{
+		id: resource.NewResourceFromMap(
+			map[string]interface{}{
+				"kind": "foo",
+				"metadata": map[string]interface{}{
+					"name": "some-name",
+				},
+				"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{}{
+									"image": "nginx",
+									"name":  "nginx",
+								},
+							},
+						},
+					},
+				},
+			}),
+	}
+	expected := resmap.ResMap{
+		id: resource.NewResourceFromMap(
+			map[string]interface{}{
+				"kind": "foo",
+				"metadata": map[string]interface{}{
+					"name": "some-name",
+				},
+				"spec": map[string]interface{}{
+					"replica": "3",
+					"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{}{
+									"image": "nginx",
+									"name":  "my-nginx",
+									"command": []interface{}{
+										"arg1",
+										"arg2",
+										"arg3",
+									},
+								},
+							},
+						},
+					},
+				},
+			}),
+	}
+	err = tr.Transform(base)
+	if err != nil {
+		t.Fatalf("unexpected error : %v", err)
+	}
+	if !reflect.DeepEqual(base, expected) {
+		err = expected.ErrorIfNotEqual(base)
+		t.Fatalf("actual doesn't match expected: %v", err)
+	}
+}
+
+func TestNewPatchJson6902FactoryMultiConflict(t *testing.T) {
+	ldr := loadertest.NewFakeLoader("/testpath")
+	operations := []byte(`[
+        {"op": "add", "path": "/spec/replica", "value": "3"}
+]`)
+	err := ldr.AddFile("/testpath/patch.json", operations)
+	if err != nil {
+		t.Fatalf("Failed to setup fake ldr.")
+	}
+
+	jsonPatches := []byte(`
+- target:
+    kind: foo
+    name: some-name
+  path: /testpath/patch.json
+
+- target:
+    kind: foo
+    name: some-name
+  jsonPatch:
+  - op: add
+    path: /spec/replica
+    value: 4
+`)
+	var p []patch.PatchJson6902
+	err = yaml.Unmarshal(jsonPatches, &p)
+	if err != nil {
+		t.Fatalf("unexpected error : %v", err)
+	}
+
+	f := NewPatchJson6902Factory(ldr)
+	tr, err := f.MakePatchJson6902Transformer(p)
+	if err != nil {
+		t.Fatalf("unexpected error : %v", err)
+	}
+	if tr == nil {
+		t.Fatal("the returned transformer should not be nil")
+	}
+
+	id := resource.NewResId(schema.GroupVersionKind{Kind: "foo"}, "some-name")
+	base := resmap.ResMap{
+		id: resource.NewResourceFromMap(
+			map[string]interface{}{
+				"kind": "foo",
+				"metadata": map[string]interface{}{
+					"name": "somename",
+				},
+				"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{}{
+									"image": "nginx",
+									"name":  "nginx",
+								},
+							},
+						},
+					},
+				},
+			}),
+	}
+
+	err = tr.Transform(base)
+	if err == nil {
+		t.Fatal("expected conflict")
+	}
+	if !strings.Contains(err.Error(), "found conflict between different patches") {
+		t.Fatalf("incorrect error happened %v", err)
 	}
-
-	_, err = f.MakePatchJson6902Transformer()
-	if err != nil {
-		t.Fatalf("unexpected error : %v", err)
-	}
-
 }

pkg/patch/transformer/patchjson6902json.go

@@ -23,7 +23,7 @@ import (
 	"github.com/kubernetes-sigs/kustomize/pkg/transformers"
 )
 
-// patchJson6902Transformer applies patches.
+// patchJson6902JSONTransformer applies patches.
 type patchJson6902JSONTransformer struct {
 	target resource.ResId
 	patch  jsonpatch.Patch

pkg/patch/transformer/patchjson6902yaml.go

@@ -24,7 +24,7 @@ import (
 	"github.com/kubernetes-sigs/kustomize/pkg/transformers"
 )
 
-// patchJson6902Transformer applies patches.
+// patchJson6902YAMLTransformer applies patches.
 type patchJson6902YAMLTransformer struct {
 	target resource.ResId
 	patch  yamlpatch.Patch

pkg/resmap/resmap.go

@@ -117,6 +117,16 @@ func (m ResMap) ErrorIfNotEqual(m2 ResMap) error {
 	return nil
 }
 
+// DeepCopy clone the resmap into a new one
+func (m ResMap) DeepCopy() ResMap {
+	mcopy := make(ResMap)
+	for id, obj := range m {
+		mcopy[id] = resource.NewResourceFromUnstruct(obj.Unstructured)
+		mcopy[id].SetBehavior(obj.Behavior())
+	}
+	return mcopy
+}
+
 func (m ResMap) insert(newName string, obj *unstructured.Unstructured) error {
 	oldName := obj.GetName()
 	gvk := obj.GroupVersionKind()

pkg/transformers/multitransformer.go

@@ -16,11 +16,16 @@ limitations under the License.
 
 package transformers
 
-import "github.com/kubernetes-sigs/kustomize/pkg/resmap"
+import (
+	"fmt"
+
+	"github.com/kubernetes-sigs/kustomize/pkg/resmap"
+)
 
 // multiTransformer contains a list of transformers.
 type multiTransformer struct {
-	transformers []Transformer
+	transformers         []Transformer
+	checkConflictEnabled bool
 }
 
 var _ Transformer = &multiTransformer{}
@@ -28,13 +33,29 @@ var _ Transformer = &multiTransformer{}
 // NewMultiTransformer constructs a multiTransformer.
 func NewMultiTransformer(t []Transformer) Transformer {
 	r := &multiTransformer{
-		transformers: make([]Transformer, len(t))}
+		transformers:         make([]Transformer, len(t)),
+		checkConflictEnabled: false}
+	copy(r.transformers, t)
+	return r
+}
+
+// NewMultiTransformerWithConflictCheck constructs a multiTransformer with checking of conflicts.
+func NewMultiTransformerWithConflictCheck(t []Transformer) Transformer {
+	r := &multiTransformer{
+		transformers:         make([]Transformer, len(t)),
+		checkConflictEnabled: true}
 	copy(r.transformers, t)
 	return r
 }
 
 // Transform prepends the name prefix.
 func (o *multiTransformer) Transform(m resmap.ResMap) error {
+	if o.checkConflictEnabled {
+		return o.transformWithCheckConflict(m)
+	}
+	return o.transform(m)
+}
+func (o *multiTransformer) transform(m resmap.ResMap) error {
 	for _, t := range o.transformers {
 		err := t.Transform(m)
 		if err != nil {
@@ -43,3 +64,30 @@ func (o *multiTransformer) Transform(m resmap.ResMap) error {
 	}
 	return nil
 }
+
+// Of the len(o.transformers)! possible transformer orderings, compare to a reversed order.
+// A spot check to perform when the transformations are supposed to be commutative.
+// Fail if there's a difference in the result.
+func (o *multiTransformer) transformWithCheckConflict(m resmap.ResMap) error {
+	mcopy := m.DeepCopy()
+	err := o.transform(m)
+	if err != nil {
+		return err
+	}
+	o.reverseTransformers()
+	err = o.transform(mcopy)
+	if err != nil {
+		return err
+	}
+	err = m.ErrorIfNotEqual(mcopy)
+	if err != nil {
+		return fmt.Errorf("found conflict between different patches\n%v", err)
+	}
+	return nil
+}
+
+func (o *multiTransformer) reverseTransformers() {
+	for i, j := 0, len(o.transformers)-1; i < j; i, j = i+1, j-1 {
+		o.transformers[i], o.transformers[j] = o.transformers[j], o.transformers[i]
+	}
+}