Merge pull request #3771 from natasha41575/ReplacementFilterTests

add more tests for replacement filter

api/filters/replacement/replacement.go

@@ -17,7 +17,6 @@ type Filter struct {
 }
 
 // Filter replaces values of targets with values from sources
-// TODO (#3492): Connect this to a replacement transformer plugin
 func (f Filter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
 	for _, r := range f.Replacements {
 		if r.Source == nil || r.Targets == nil {
@@ -37,6 +36,9 @@ func (f Filter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
 
 func applyReplacement(nodes []*yaml.RNode, value *yaml.RNode, targets []*types.TargetSelector) ([]*yaml.RNode, error) {
 	for _, t := range targets {
+		if t.Select == nil {
+			return nil, fmt.Errorf("target must specify resources to select")
+		}
 		if len(t.FieldPaths) == 0 {
 			t.FieldPaths = []string{types.DefaultReplacementFieldPath}
 		}
@@ -54,13 +56,8 @@ func applyReplacement(nodes []*yaml.RNode, value *yaml.RNode, targets []*types.T
 }
 
 func applyToNode(node *yaml.RNode, value *yaml.RNode, target *types.TargetSelector) error {
-	if target.Select == nil {
-		return fmt.Errorf("target must specify resources to select")
-	}
 	for _, fp := range target.FieldPaths {
-		fieldPath := strings.Split(fp, ".")
+		t, err := node.Pipe(yaml.Lookup(strings.Split(fp, ".")...))
-		// TODO (#3492): Add tests for map keys in the fieldPath (e.g. .spec.containers[name=nginx])
-		t, err := node.Pipe(yaml.Lookup(fieldPath...))
 		if err != nil {
 			return err
 		}
@@ -83,7 +80,6 @@ func getReplacement(nodes []*yaml.RNode, r *types.Replacement) (*yaml.RNode, err
 	}
 	fieldPath := strings.Split(r.Source.FieldPath, ".")
 
-	// TODO (#3492): Add tests for map keys in the fieldPath (e.g. .spec.containers[name=nginx])
 	rn, err := source.Pipe(yaml.Lookup(fieldPath...))
 	if err != nil {
 		return nil, err
@@ -111,10 +107,17 @@ func selectSourceNode(nodes []*yaml.RNode, selector *types.SourceSelector) (*yam
 }
 
 func getKrmId(n *yaml.RNode) *types.KrmId {
-	ns, _ := n.GetNamespace()
+	ns, err := n.GetNamespace()
-	apiVersion := yaml.GetValue(n.Field(yaml.APIVersionField).Value)
+	if err != nil {
-	group, version := resid.ParseGroupVersion(apiVersion)
+		// Resource has no metadata (no apiVersion, kind, nor metadata field).
-
+		// In this case, it cannot be selected.
+		return &types.KrmId{}
+	}
+	apiVersion := n.Field(yaml.APIVersionField)
+	var group, version string
+	if apiVersion != nil {
+		group, version = resid.ParseGroupVersion(yaml.GetValue(apiVersion.Value))
+	}
 	return &types.KrmId{
 		Gvk:       resid.Gvk{Group: group, Version: version, Kind: n.GetKind()},
 		Name:      n.GetName(),

api/filters/replacement/replacement_test.go

@@ -17,7 +17,7 @@ func TestFilter(t *testing.T) {
 		input        string
 		replacements string
 		expected     string
-		expectedErr  bool
+		expectedErr  string
 	}{
 		"simple": {
 			input: `apiVersion: v1
@@ -45,6 +45,19 @@ spec:
         name: nginx-tagged
       - image: postgres:1.8.0
         name: postgresdb
+---
+apiVersion: v1
+kind: Deployment
+metadata:
+  name: deploy3
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
 `,
 			replacements: `replacements:
 - source:
@@ -83,6 +96,19 @@ spec:
         name: nginx-tagged
       - image: postgres:1.8.0
         name: postgresdb
+---
+apiVersion: v1
+kind: Deployment
+metadata:
+  name: deploy3
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
 `,
 		},
 		"complex type": {
@@ -295,7 +321,7 @@ spec:
   - select:
       kind: Deployment
 `,
-			expectedErr: true,
+			expectedErr: "more than one match for source ~G_~V_Deployment",
 		},
 		"replacement has no source": {
 			input: `apiVersion: v1
@@ -316,7 +342,246 @@ spec:
   - select:
       kind: Deployment
 `,
-			expectedErr: true,
+			expectedErr: "replacements must specify a source and at least one target",
+		},
+		"field paths with key-value pairs": {
+			input: `apiVersion: v1
+kind: Deployment
+metadata:
+  name: deploy1
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+---
+apiVersion: v1
+kind: Deployment
+metadata:
+  name: deploy2
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+`,
+			replacements: `replacements:
+- source:
+    kind: Deployment
+    name: deploy2
+    fieldPath: spec.template.spec.containers.[name=nginx-tagged].image
+  targets:
+  - select:
+      kind: Deployment
+      name: deploy1
+    fieldPaths: 
+    - spec.template.spec.containers.[name=postgresdb].image
+`,
+			expected: `apiVersion: v1
+kind: Deployment
+metadata:
+  name: deploy1
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: nginx:1.7.9
+        name: postgresdb
+---
+apiVersion: v1
+kind: Deployment
+metadata:
+  name: deploy2
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+`,
+		},
+		"select by group and version": {
+			input: `apiVersion: my-group-1/v1
+kind: Custom
+metadata:
+  name: my-name
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+---
+apiVersion: my-group-2/v2
+kind: Custom
+metadata:
+  name: my-name
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+---
+apiVersion: my-group-3/v3
+kind: Custom
+metadata:
+  name: my-name
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+`,
+			replacements: `replacements:
+- source:
+    group: my-group-2
+    fieldPath: spec.template.spec.containers.0.image
+  targets:
+  - select:
+      version: v3
+    fieldPaths: 
+    - spec.template.spec.containers.1.image
+`,
+			expected: `apiVersion: my-group-1/v1
+kind: Custom
+metadata:
+  name: my-name
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+---
+apiVersion: my-group-2/v2
+kind: Custom
+metadata:
+  name: my-name
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+---
+apiVersion: my-group-3/v3
+kind: Custom
+metadata:
+  name: my-name
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: nginx:1.7.9
+        name: postgresdb
+`,
+		},
+		// regression test for missing metadata handling
+		"missing metadata": {
+			input: `spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+---
+apiVersion: my-group/v1
+kind: Custom
+metadata:
+  name: my-name-1
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+---
+apiVersion: my-group/v1
+kind: Custom
+metadata:
+  name: my-name-2
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+`,
+			replacements: `replacements:
+- source:
+    name: my-name-1
+    fieldPath: spec.template.spec.containers.0.image
+  targets:
+  - select:
+      name: my-name-2
+    fieldPaths: 
+    - spec.template.spec.containers.1.image
+`,
+			expected: `spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+---
+apiVersion: my-group/v1
+kind: Custom
+metadata:
+  name: my-name-1
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: postgres:1.8.0
+        name: postgresdb
+---
+apiVersion: my-group/v1
+kind: Custom
+metadata:
+  name: my-name-2
+spec:
+  template:
+    spec:
+      containers:
+      - image: nginx:1.7.9
+        name: nginx-tagged
+      - image: nginx:1.7.9
+        name: postgresdb
+`,
 		},
 	}
 
@@ -324,11 +589,20 @@ spec:
 		t.Run(tn, func(t *testing.T) {
 			f := Filter{}
 			err := yaml.Unmarshal([]byte(tc.replacements), &f)
-			assert.NoError(t, err)
+			if !assert.NoError(t, err) {
+				t.FailNow()
+			}
 			actual, err := filtertest.RunFilterE(t, tc.input, f)
-			assert.Equal(t, tc.expectedErr, err != nil)
+			if err != nil {
-			if !tc.expectedErr &&
+				if tc.expectedErr == "" {
-				!assert.Equal(t, strings.TrimSpace(tc.expected), strings.TrimSpace(actual)) {
+					t.Errorf("unexpected error: %s\n", err.Error())
+					t.FailNow()
+				}
+				if !assert.Equal(t, tc.expectedErr, err.Error()) {
+					t.FailNow()
+				}
+			}
+			if !assert.Equal(t, strings.TrimSpace(tc.expected), strings.TrimSpace(actual)) {
 				t.FailNow()
 			}
 		})

api/types/replacement.go

@@ -23,7 +23,7 @@ type SourceSelector struct {
 	// Structured field path expected in the allowed object.
 	FieldPath string `json:"fieldPath" yaml:"fieldPath"`
 
-	// Used to refine the interpretation of the field
+	// Used to refine the interpretation of the field.
 	Options *FieldOptions `json:"options" yaml:"options"`
 }
 
@@ -33,20 +33,20 @@ type TargetSelector struct {
 	Select *Selector `json:"select" yaml:"select"`
 
 	// From the allowed set, remove objects that match this.
-	// TODO (#3492): Remove matches listed in the exclude field
+	// TODO (#3492): Remove matches listed in the `reject` field
 	//  Currently this field is unused
 	Reject *Selector `json:"reject" yaml:"reject"`
 
 	// Structured field paths expected in each allowed object.
 	FieldPaths []string `json:"fieldPaths" yaml:"fieldPaths"`
 
-	// Used to refine the interpretation of the field
+	// Used to refine the interpretation of the field.
 	Options *FieldOptions `json:"options" yaml:"options"`
 }
 
-// FieldPath is a structured field path to the desired object
+// FieldOptions refine the interpretation of FieldPaths.
 // TODO (#3492): Implement use of these options, they are
-//  currently used
+//  currently unused
 type FieldOptions struct {
 	// Used to split/join the field.
 	Delimiter string `json:"delimiter" yaml:"delimiter"`
@@ -54,9 +54,9 @@ type FieldOptions struct {
 	// Which position in the split to consider.
 	Index int `json:"index" yaml:"index"`
 
-	// None, Base64, URL, Hex, etc
+	// None, Base64, URL, Hex, etc.
 	Encoding string `json:"encoding" yaml:"index"`
 
-	// If field missing, add it
+	// If field missing, add it.
 	Create bool `json:"create" yaml:"create"`
 }