More fieldspec tests.

api/filters/fieldspec/fieldspec.go

@@ -4,6 +4,7 @@
 package fieldspec
 
 import (
+	"fmt"
 	"strings"
 
 	"sigs.k8s.io/kustomize/api/filters/filtersutil"
@@ -15,7 +16,16 @@ import (
 
 var _ yaml.Filter = Filter{}
 
-// Filter applies a single fieldSpec to a single object
+// Filter possibly mutates its object argument using a FieldSpec.
+// If the object matches the FieldSpec, and the node found
+// by following the fieldSpec's path is non-null, this filter calls
+// the setValue function on the node at the end of the path.
+// If any part of the path doesn't exist, the filter returns
+// without doing anything and without error, unless it was set
+// to create the path. If set to create, it creates a tree of maps
+// along the path, and the leaf node gets the setValue called on it.
+// Error on GVK mismatch, empty or poorly formed path.
+// Filter expect kustomize style paths, not JSON paths.
 // Filter stores internal state and should not be reused
 type Filter struct {
 	// FieldSpec contains the path to the value to set.
@@ -39,7 +49,8 @@ func (fltr Filter) Filter(obj *yaml.RNode) (*yaml.RNode, error) {
 		return obj, errors.Wrap(err)
 	}
 	fltr.path = utils.PathSplitter(fltr.FieldSpec.Path)
-	if err := fltr.filter(obj); err != nil {
+	err := fltr.filter(obj)
+	if err != nil {
 		s, _ := obj.String()
 		return nil, errors.WrapPrefixf(err,
 			"considering field '%s' of object\n%v", fltr.FieldSpec.Path, s)
@@ -47,6 +58,7 @@ func (fltr Filter) Filter(obj *yaml.RNode) (*yaml.RNode, error) {
 	return obj, nil
 }
 
+// Recursively called.
 func (fltr Filter) filter(obj *yaml.RNode) error {
 	if len(fltr.path) == 0 {
 		// found the field -- set its value
@@ -57,9 +69,9 @@ func (fltr Filter) filter(obj *yaml.RNode) error {
 	}
 	switch obj.YNode().Kind {
 	case yaml.SequenceNode:
-		return fltr.seq(obj)
+		return fltr.handleSequence(obj)
 	case yaml.MappingNode:
-		return fltr.field(obj)
+		return fltr.handleMap(obj)
 	case yaml.AliasNode:
 		return fltr.filter(yaml.NewRNode(obj.YNode().Alias))
 	default:
@@ -67,17 +79,20 @@ func (fltr Filter) filter(obj *yaml.RNode) error {
 	}
 }
 
-// field calls filter on the field matching the next path element
+// handleMap calls filter on the map field matching the next path element
-func (fltr Filter) field(obj *yaml.RNode) error {
+func (fltr Filter) handleMap(obj *yaml.RNode) error {
 	fieldName, isSeq := isSequenceField(fltr.path[0])
+	if fieldName == "" {
+		return fmt.Errorf("cannot set or create an empty field name")
+	}
 	// lookup the field matching the next path element
-	var lookupField yaml.Filter
+	var operation yaml.Filter
 	var kind yaml.Kind
 	tag := yaml.NodeTagEmpty
 	switch {
 	case !fltr.FieldSpec.CreateIfNotPresent || fltr.CreateKind == 0 || isSeq:
-		// dont' create the field if we don't find it
+		// don't create the field if we don't find it
-		lookupField = yaml.Lookup(fieldName)
+		operation = yaml.Lookup(fieldName)
 		if isSeq {
 			// The query path thinks this field should be a sequence;
 			// accept this hint for use later if the tag is NodeTagNull.
@@ -85,21 +100,25 @@ func (fltr Filter) field(obj *yaml.RNode) error {
 		}
 	case len(fltr.path) <= 1:
 		// create the field if it is missing: use the provided node kind
-		lookupField = yaml.LookupCreate(fltr.CreateKind, fieldName)
+		operation = yaml.LookupCreate(fltr.CreateKind, fieldName)
 		kind = fltr.CreateKind
 		tag = fltr.CreateTag
 	default:
 		// create the field if it is missing: must be a mapping node
-		lookupField = yaml.LookupCreate(yaml.MappingNode, fieldName)
+		operation = yaml.LookupCreate(yaml.MappingNode, fieldName)
 		kind = yaml.MappingNode
 		tag = yaml.NodeTagMap
 	}
 
 	// locate (or maybe create) the field
-	field, err := obj.Pipe(lookupField)
+	field, err := obj.Pipe(operation)
-	if err != nil || field == nil {
+	if err != nil {
 		return errors.WrapPrefixf(err, "fieldName: %s", fieldName)
 	}
+	if field == nil {
+		// No error if field not found.
+		return nil
+	}
 
 	// if the value exists, but is null and kind is set,
 	// then change it to the creation type
@@ -117,7 +136,7 @@ func (fltr Filter) field(obj *yaml.RNode) error {
 }
 
 // seq calls filter on all sequence elements
-func (fltr Filter) seq(obj *yaml.RNode) error {
+func (fltr Filter) handleSequence(obj *yaml.RNode) error {
 	if err := obj.VisitElements(func(node *yaml.RNode) error {
 		// recurse on each element -- re-allocating a Filter is
 		// not strictly required, but is more consistent with field
@@ -128,16 +147,14 @@ func (fltr Filter) seq(obj *yaml.RNode) error {
 		return errors.WrapPrefixf(err,
 			"visit traversal on path: %v", fltr.path)
 	}
-
 	return nil
 }
 
 // isSequenceField returns true if the path element is for a sequence field.
 // isSequence also returns the path element with the '[]' suffix trimmed
 func isSequenceField(name string) (string, bool) {
-	isSeq := strings.HasSuffix(name, "[]")
+	shorter := strings.TrimSuffix(name, "[]")
-	name = strings.TrimSuffix(name, "[]")
+	return shorter, shorter != name
-	return name, isSeq
 }
 
 // isMatchGVK returns true if the fs.GVK matches the obj GVK.

api/filters/fieldspec/fieldspec_test.go

@@ -15,18 +15,60 @@ import (
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
 
-type TestCase struct {
+func TestFilter_Filter(t *testing.T) {
-	name      string
+	testCases := map[string]struct {
 		input     string
 		expected  string
 		filter    fieldspec.Filter
 		fieldSpec string
 		error     string
-}
+	}{
+		"path not found": {
+			fieldSpec: `
+path: a/b
+group: foo
+kind: Bar
+`,
+			input: `
+apiVersion: foo
+kind: Bar
+xxx:
+`,
+			expected: `
+apiVersion: foo
+kind: Bar
+xxx:
+`,
+			filter: fieldspec.Filter{
+				SetValue: filtersutil.SetScalar("e"),
+			},
+		},
+		"empty path": {
+			fieldSpec: `
+group: foo
+kind: Bar
+`,
+			input: `
+apiVersion: foo
+kind: Bar
+xxx:
+`,
+			expected: `
+apiVersion: foo
+kind: Bar
+xxx:
+`,
+			error: `considering field '' of object
+apiVersion: foo
+kind: Bar
+xxx:
+: cannot set or create an empty field name`,
+			filter: fieldspec.Filter{
+				SetValue: filtersutil.SetScalar("e"),
+			},
+		},
 
-var tests = []TestCase{
+		"update": {
-	{
-		name: "update",
 			fieldSpec: `
 path: a/b
 group: foo
@@ -49,8 +91,7 @@ a:
 			},
 		},
 
-	{
+		"update-kind-not-match": {
-		name: "update-kind-not-match",
 			fieldSpec: `
 path: a/b
 group: foo
@@ -73,8 +114,7 @@ a:
 			},
 		},
 
-	{
+		"update-group-not-match": {
-		name: "update-group-not-match",
 			fieldSpec: `
 path: a/b
 group: foo1
@@ -97,8 +137,7 @@ a:
 			},
 		},
 
-	{
+		"update-version-not-match": {
-		name: "update-version-not-match",
 			fieldSpec: `
 path: a/b
 group: foo
@@ -122,8 +161,7 @@ a:
 			},
 		},
 
-	{
+		"bad-version": {
-		name: "bad-version",
 			fieldSpec: `
 path: a/b
 group: foo
@@ -147,8 +185,7 @@ a:
 			},
 		},
 
-	{
+		"bad-meta": {
-		name: "bad-meta",
 			fieldSpec: `
 path: a/b
 group: foo
@@ -165,8 +202,7 @@ a:
 			error: "missing Resource metadata",
 		},
 
-	{
+		"miss-match-type": {
-		name: "miss-match-type",
 			fieldSpec: `
 path: a/b/c
 kind: Bar
@@ -186,8 +222,7 @@ a:
 			},
 		},
 
-	{
+		"add": {
-		name: "add",
 			fieldSpec: `
 path: a/b/c/d
 group: foo
@@ -210,8 +245,7 @@ a: {b: {c: {d: e}}}
 			},
 		},
 
-	{
+		"update-in-sequence": {
-		name: "update-in-sequence",
 			fieldSpec: `
 path: a/b[]/c/d
 group: foo
@@ -239,8 +273,7 @@ a:
 		},
 
 		// Don't create a sequence
-	{
+		"empty-sequence-no-create": {
-		name: "empty-sequence-no-create",
 			fieldSpec: `
 path: a/b[]/c/d
 group: foo
@@ -264,8 +297,7 @@ a: {}
 		},
 
 		// Create a new field for an element in a sequence
-	{
+		"empty-sequence-create": {
-		name: "empty-sequence-create",
 			fieldSpec: `
 path: a/b[]/c/d
 group: foo
@@ -292,8 +324,7 @@ a:
 			},
 		},
 
-	{
+		"group v1": {
-		name: "group v1",
 			fieldSpec: `
 path: a/b
 group: v1
@@ -314,8 +345,7 @@ kind: Bar
 			},
 		},
 
-	{
+		"version v1": {
-		name: "version v1",
 			fieldSpec: `
 path: a/b
 version: v1
@@ -337,8 +367,8 @@ a:
 				CreateKind: yaml.ScalarNode,
 			},
 		},
-	{
+
-		name: "successfully set field on array entry no sequence hint",
+		"successfully set field on array entry no sequence hint": {
 			fieldSpec: `
 path: spec/containers/image
 version: v1
@@ -363,8 +393,8 @@ spec:
 				CreateKind: yaml.ScalarNode,
 			},
 		},
-	{
+
-		name: "successfully set field on array entry with sequence hint",
+		"successfully set field on array entry with sequence hint": {
 			fieldSpec: `
 path: spec/containers[]/image
 version: v1
@@ -389,8 +419,7 @@ spec:
 				CreateKind: yaml.ScalarNode,
 			},
 		},
-	{
+		"failure to set field on array entry with sequence hint in path": {
-		name: "failure to set field on array entry with sequence hint in path",
 			fieldSpec: `
 path: spec/containers[]/image
 version: v1
@@ -413,8 +442,8 @@ spec:
 				CreateKind: yaml.ScalarNode,
 			},
 		},
-	{
+
-		name: "failure to set field on array entry, no sequence hint in path",
+		"failure to set field on array entry, no sequence hint in path": {
 			fieldSpec: `
 path: spec/containers/image
 version: v1
@@ -437,8 +466,7 @@ spec:
 				CreateKind: yaml.ScalarNode,
 			},
 		},
-	{
+		"fieldname with slash '/'": {
-		name: "filedname with slash '/'",
 			fieldSpec: `
 path: a/b\/c/d
 version: v1
@@ -463,8 +491,7 @@ a:
 				CreateKind: yaml.ScalarNode,
 			},
 		},
-	{
+		"fieldname with multiple '/'": {
-		name: "filedname with multiple '/'",
 			fieldSpec: `
 path: a/b\/c/d\/e/f
 version: v1
@@ -493,18 +520,17 @@ a:
 		},
 	}
 
-func TestFilter_Filter(t *testing.T) {
+	for n := range testCases {
-	for i := range tests {
+		tc := testCases[n]
-		test := tests[i]
+		t.Run(n, func(t *testing.T) {
-		t.Run(test.name, func(t *testing.T) {
+			err := yaml.Unmarshal([]byte(tc.fieldSpec), &tc.filter.FieldSpec)
-			err := yaml.Unmarshal([]byte(test.fieldSpec), &test.filter.FieldSpec)
 			if !assert.NoError(t, err) {
 				t.FailNow()
 			}
 
 			out := &bytes.Buffer{}
 			rw := &kio.ByteReadWriter{
-				Reader:                bytes.NewBufferString(test.input),
+				Reader:                bytes.NewBufferString(tc.input),
 				Writer:                out,
 				OmitReaderAnnotations: true,
 			}
@@ -512,11 +538,11 @@ func TestFilter_Filter(t *testing.T) {
 			// run the filter
 			err = kio.Pipeline{
 				Inputs:  []kio.Reader{rw},
-				Filters: []kio.Filter{kio.FilterAll(test.filter)},
+				Filters: []kio.Filter{kio.FilterAll(tc.filter)},
 				Outputs: []kio.Writer{rw},
 			}.Execute()
-			if test.error != "" {
+			if tc.error != "" {
-				if !assert.EqualError(t, err, test.error) {
+				if !assert.EqualError(t, err, tc.error) {
 					t.FailNow()
 				}
 				// stop rest of test
@@ -529,7 +555,7 @@ func TestFilter_Filter(t *testing.T) {
 
 			// check results
 			if !assert.Equal(t,
-				strings.TrimSpace(test.expected),
+				strings.TrimSpace(tc.expected),
 				strings.TrimSpace(out.String())) {
 				t.FailNow()
 			}

api/internal/accumulator/namereferencetransformer.go

@@ -17,6 +17,8 @@ type nameReferenceTransformer struct {
 	backRefs []builtinconfig.NameBackReferences
 }
 
+const doDebug = false
+
 var _ resmap.Transformer = &nameReferenceTransformer{}
 
 type filterMap map[*resource.Resource][]nameref.Filter
@@ -47,7 +49,7 @@ func newNameReferenceTransformer(
 //
 func (t *nameReferenceTransformer) Transform(m resmap.ResMap) error {
 	fMap := t.determineFilters(m.Resources())
-	// debug(fMap)
+	debug(fMap)
 	for r, fList := range fMap {
 		c := m.SubsetThatCouldBeReferencedByResource(r)
 		for _, f := range fList {
@@ -61,8 +63,10 @@ func (t *nameReferenceTransformer) Transform(m resmap.ResMap) error {
 	return nil
 }
 
-//nolint: deadcode, unused
 func debug(fMap filterMap) {
+	if !doDebug {
+		return
+	}
 	fmt.Printf("filterMap has %d entries:\n", len(fMap))
 	rCount := 0
 	for r, fList := range fMap {
@@ -117,6 +121,9 @@ func (t *nameReferenceTransformer) determineFilters(
 						// that might need updating.
 						fMap[res] = append(fMap[res], nameref.Filter{
 							// Name field to write in the Referrer.
+							// If the path specified here isn't found in
+							// the Referrer, nothing happens (no error,
+							// no field creation).
 							NameFieldToUpdate: referrerSpec,
 							// Specification of object class to read from.
 							// Always read from metadata/name field.