Update patch factory and add multi transformer with checking conflicts

kubernetes-sigs · Sep 4, 2018 · d782abb · d782abb
1 parent 95f5bec
commit d782abb
Show file tree

Hide file tree

Showing 6 changed files with 287 additions and 39 deletions.
diff --git a/pkg/patch/transformer/factory.go b/pkg/patch/transformer/factory.go
@@ -20,24 +20,39 @@ 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"
 	"github.com/kubernetes-sigs/kustomize/pkg/transformers"
 	"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 nil, nil
-}
+		return newPatchJson6902JSONTransformer(targetId, decodedPatch)
 
-// 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)
+	return nil, nil
 }
diff --git a/pkg/patch/transformer/factory_test.go b/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 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 f == nil {
-		t.Fatalf("the returned factory shouldn't be nil ")
+	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)
 	}
 
-	_, err = f.MakePatchJson6902Transformer()
+	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)
+	}
 }
diff --git a/pkg/patch/transformer/patchjson6902json.go b/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

diff --git a/pkg/patch/transformer/patchjson6902yaml.go b/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

diff --git a/pkg/resmap/resmap.go b/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()