Simplify callback bookkeeping

This commit removes an unnecessary indirection through a closure.

Signed-off-by: Michael Bridgen <michael@weave.works>

pkg/update/filter.go

@@ -28,11 +28,11 @@ import (
 // [kyaml](https://github.com/kubernetes-sigs/kustomize/blob/kyaml/v0.10.16/kyaml/setters2/set.go),
 // with the following changes:
 //
-// - it records each field it sets
+// - it calls its callback for each field it sets
 //
-// - it will implicitly set all fields referring to a setter in the
-// schema -- this is a flag in the kyaml implementation, but the only
-// desired mode of operation here
+// - it will set all fields referring to a setter present in the
+// schema -- this is behind a flag in the kyaml implementation, but
+// the only desired mode of operation here
 //
 // - substitutions are not supported -- they are not used for image
 // updates
@@ -45,12 +45,12 @@ import (
 // - only per-field schema references (those in a comment in the YAML)
 // are considered -- these are the only ones relevant to image updates
 
-type SetAllAndRecord struct {
+type SetAllCallback struct {
 	SettersSchema *spec.Schema
-	Record        func(setter, oldValue, newValue string)
+	Callback      func(setter, oldValue, newValue string)
 }
 
-func (s *SetAllAndRecord) Filter(object *yaml.RNode) (*yaml.RNode, error) {
+func (s *SetAllCallback) Filter(object *yaml.RNode) (*yaml.RNode, error) {
 	return object, accept(s, object, "", s.SettersSchema)
 }
 
@@ -104,7 +104,7 @@ func accept(v visitor, object *yaml.RNode, p string, settersSchema *spec.Schema)
 }
 
 // set applies the value from ext to field
-func (s *SetAllAndRecord) set(field *yaml.RNode, ext *setters2.CliExtension, sch *spec.Schema) (bool, error) {
+func (s *SetAllCallback) set(field *yaml.RNode, ext *setters2.CliExtension, sch *spec.Schema) (bool, error) {
 	// check full setter
 	if ext.Setter == nil {
 		return false, nil
@@ -113,19 +113,18 @@ func (s *SetAllAndRecord) set(field *yaml.RNode, ext *setters2.CliExtension, sch
 	// this has a full setter, set its value
 	old := field.YNode().Value
 	field.YNode().Value = ext.Setter.Value
-	s.Record(ext.Setter.Name, old, ext.Setter.Value)
+	s.Callback(ext.Setter.Name, old, ext.Setter.Value)
 
 	// format the node so it is quoted if it is a string. If there is
-	// type information on the setter schema, we use it. Otherwise we
-	// fall back to the field schema if it exists.
+	// type information on the setter schema, we use it.
 	if len(sch.Type) > 0 {
 		yaml.FormatNonStringStyle(field.YNode(), *sch)
 	}
 	return true, nil
 }
 
 // visitScalar
-func (s *SetAllAndRecord) visitScalar(object *yaml.RNode, p string, fieldSchema *openapi.ResourceSchema) error {
+func (s *SetAllCallback) visitScalar(object *yaml.RNode, p string, fieldSchema *openapi.ResourceSchema) error {
 	if fieldSchema == nil {
 		return nil
 	}
@@ -139,7 +138,6 @@ func (s *SetAllAndRecord) visitScalar(object *yaml.RNode, p string, fieldSchema
 	}
 
 	// perform a direct set of the field if it matches
-	// %%% FIXME record it!
 	_, err = s.set(object, ext, fieldSchema.Schema)
 	return err
 }

pkg/update/setters.go

@@ -94,31 +94,40 @@ func UpdateWithSetters(inpath, outpath string, policies []imagev1alpha1_reflect.
 	result := Result{
 		Files: make(map[string]FileResult),
 	}
-	callbacks := make(map[string]func(string, *yaml.RNode))
-	makeCallback := func(ref imageRef) func(string, *yaml.RNode) {
-		return func(file string, node *yaml.RNode) {
-			meta, err := node.GetMeta()
-			if err != nil {
-				return
-			}
-			oid := ObjectIdentifier{meta.GetIdentifier()}
 
-			fileres, ok := result.Files[file]
-			if !ok {
-				fileres = FileResult{
-					Objects: make(map[ObjectIdentifier][]ImageRef),
-				}
-				result.Files[file] = fileres
+	// Compilng the result needs the file, the image ref used, and the
+	// object. Each setter will supply its own name to its callback,
+	// which can be used to look up the image ref; the file and object
+	// we will get from `setAll` which keeps track of those as it
+	// iterates.
+	imageRefs := make(map[string]imageRef)
+	setAllCallback := func(file, setterName string, node *yaml.RNode) {
+		ref, ok := imageRefs[setterName]
+		if !ok {
+			return
+		}
+
+		meta, err := node.GetMeta()
+		if err != nil {
+			return
+		}
+		oid := ObjectIdentifier{meta.GetIdentifier()}
+
+		fileres, ok := result.Files[file]
+		if !ok {
+			fileres = FileResult{
+				Objects: make(map[ObjectIdentifier][]ImageRef),
 			}
-			objres, ok := fileres.Objects[oid]
-			for _, n := range objres {
-				if n == ref {
-					return
-				}
+			result.Files[file] = fileres
+		}
+		objres, ok := fileres.Objects[oid]
+		for _, n := range objres {
+			if n == ref {
+				return
 			}
-			objres = append(objres, ref)
-			fileres.Objects[oid] = objres
 		}
+		objres = append(objres, ref)
+		fileres.Objects[oid] = objres
 	}
 
 	defs := map[string]spec.Schema{}
@@ -145,8 +154,6 @@ func UpdateWithSetters(inpath, outpath string, policies []imagev1alpha1_reflect.
 			},
 		}
 
-		record := makeCallback(ref)
-
 		tag := ref.Identifier()
 		// annoyingly, neither the library imported above, nor an
 		// alternative I found, will yield the original image name;
@@ -155,20 +162,20 @@ func UpdateWithSetters(inpath, outpath string, policies []imagev1alpha1_reflect.
 
 		imageSetter := fmt.Sprintf("%s:%s", policy.GetNamespace(), policy.GetName())
 		defs[fieldmeta.SetterDefinitionPrefix+imageSetter] = setterSchema(imageSetter, policy.Status.LatestImage)
-		callbacks[imageSetter] = record
+		imageRefs[imageSetter] = ref
 
 		tagSetter := imageSetter + ":tag"
 		defs[fieldmeta.SetterDefinitionPrefix+tagSetter] = setterSchema(tagSetter, tag)
-		callbacks[tagSetter] = record
+		imageRefs[tagSetter] = ref
 
 		// Context().Name() gives the image repository _as supplied_
 		nameSetter := imageSetter + ":name"
 		defs[fieldmeta.SetterDefinitionPrefix+nameSetter] = setterSchema(nameSetter, name)
-		callbacks[nameSetter] = record
+		imageRefs[nameSetter] = ref
 	}
 
 	settersSchema.Definitions = defs
-	set := &SetAllAndRecord{
+	set := &SetAllCallback{
 		SettersSchema: &settersSchema,
 	}
 
@@ -185,13 +192,7 @@ func UpdateWithSetters(inpath, outpath string, policies []imagev1alpha1_reflect.
 		Inputs:  []kio.Reader{reader},
 		Outputs: []kio.Writer{writer},
 		Filters: []kio.Filter{
-			setAll(set, func(file, setterName string, node *yaml.RNode) {
-				callback, ok := callbacks[setterName]
-				if !ok {
-					return
-				}
-				callback(file, node)
-			}),
+			setAll(set, setAllCallback),
 		},
 	}
 
@@ -203,13 +204,13 @@ func UpdateWithSetters(inpath, outpath string, policies []imagev1alpha1_reflect.
 	return result, nil
 }
 
-// setAll returns a kio.Filter using the supplied SetAllAndRecord
+// setAll returns a kio.Filter using the supplied SetAllCallback
 // (dealing with individual nodes), amd calling the given callback
 // whenever a field value is changed, and returning only nodes from
 // files with changed nodes. This is based on
 // [`SetAll`](https://github.com/kubernetes-sigs/kustomize/blob/kyaml/v0.10.16/kyaml/setters2/set.go#L503
 // from kyaml/kio.
-func setAll(filter *SetAllAndRecord, callback func(file, setterName string, node *yaml.RNode)) kio.Filter {
+func setAll(filter *SetAllCallback, callback func(file, setterName string, node *yaml.RNode)) kio.Filter {
 	return kio.FilterFunc(
 		func(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
 			filesToUpdate := sets.String{}
@@ -219,7 +220,7 @@ func setAll(filter *SetAllAndRecord, callback func(file, setterName string, node
 					return nil, err
 				}
 
-				filter.Record = func(setter, oldValue, newValue string) {
+				filter.Callback = func(setter, oldValue, newValue string) {
 					if newValue != oldValue {
 						callback(path, setter, nodes[i])
 						filesToUpdate.Insert(path)