Merge pull request #2848 from phanimarupaka/UpdateArraySetterComments

Update setter comments correctly on updates

kyaml/yaml/fns.go

@@ -447,6 +447,9 @@ type FieldSetter struct {
 	// value on the ScalarNode.
 	Name string `yaml:"name,omitempty"`
 
+	// Comments for the field
+	Comments Comments `yaml:"comments,omitempty"`
+
 	// Value is the value to set.
 	// Optional if Kind is set.
 	Value *RNode `yaml:"value,omitempty"`
@@ -504,7 +507,8 @@ func (s FieldSetter) Filter(rn *RNode) (*RNode, error) {
 
 	// create the field
 	rn.YNode().Content = append(rn.YNode().Content,
-		&yaml.Node{Kind: yaml.ScalarNode, Value: s.Name},
+		&yaml.Node{Kind: yaml.ScalarNode, HeadComment: s.Comments.HeadComment,
+			LineComment: s.Comments.LineComment, FootComment: s.Comments.FootComment, Value: s.Name},
 		s.Value.YNode())
 	return s.Value, nil
 }

kyaml/yaml/merge3/list_test.go

@@ -55,12 +55,108 @@ list: # new value
 apiVersion: apps/v1`,
 		expected: `
 apiVersion: apps/v1
-list:
+list: # new value
 - 2
 - 3
 - 4
 `},
 
+	//
+	// Test Case
+	//
+	{
+		description: `Update comment`,
+		origin: `
+list: # comment
+- 1
+- 2
+- 3`,
+		update: `
+list: # updated comment
+- 2
+- 3
+- 4`,
+		local: `
+list: # comment
+- 1
+- 2
+- 3`,
+		expected: `
+list: # updated comment
+- 2
+- 3
+- 4`},
+
+	//
+	// Test Case
+	//
+	{
+		description: `Don't update local modified comment`,
+		origin: `
+list: # origin comment
+- 1
+- 2
+- 3`,
+		update: `
+list: # updated comment
+- 2
+- 3
+- 4`,
+		local: `
+list: # local comment
+- 1
+- 2
+- 3`,
+		expected: `
+list: # local comment
+- 2
+- 3
+- 4`},
+
+	//
+	// Test Case
+	//
+	{
+		description: `Don't add local deleted comment`,
+		origin: `
+list: # origin comment
+- 1
+- 2
+- 3`,
+		update: `
+list: # updated comment
+- 2
+- 3
+- 4`,
+		local: `
+list:
+- 1
+- 2
+- 3`,
+		expected: `
+list:
+- 2
+- 3
+- 4`},
+
+	{
+		description: `Add update with comment`,
+		origin: `
+apiVersion: apps/v1
+`,
+		update: `
+list: # updated comment
+- 2
+- 3
+- 4`,
+		local: `
+apiVersion: apps/v1`,
+		expected: `
+list: # updated comment
+- 2
+- 3
+- 4`},
+
 	//
 	// Test Case
 	//
@@ -140,14 +236,14 @@ list: # ignore value
 - 3`,
 		local: `
 apiVersion: apps/v1
-list:
+list: # local comment
 - 2
 - 3
 - 4
 `,
 		expected: `
 apiVersion: apps/v1
-list:
+list: # local comment
 - 2
 - 3
 - 4

kyaml/yaml/merge3/merge3.go

@@ -15,6 +15,7 @@ func Merge(dest, original, update *yaml.RNode) (*yaml.RNode, error) {
 
 	return walk.Walker{
 		Visitor:            Visitor{},
+		VisitKeysAsScalars: true,
 		Sources:            []*yaml.RNode{dest, original, update}}.Walk()
 }
 
@@ -35,6 +36,7 @@ func MergeStrings(dest, original, update string, infer bool) (string, error) {
 	result, err := walk.Walker{
 		InferAssociativeLists: infer,
 		Visitor:               Visitor{},
+		VisitKeysAsScalars:    true,
 		Sources:               []*yaml.RNode{d, srcOriginal, srcUpdated}}.Walk()
 	if err != nil {
 		return "", err

kyaml/yaml/merge3/scalar_test.go

@@ -33,7 +33,7 @@ kind: StatefulSet # new value`},
 	// Test Case
 	//
 	{
-		description: `Ensure comments are updated`,
+		description: `Ensure comments are added`,
 		origin: `
 apiVersion: apps/v1
 kind: Deployment
@@ -81,6 +81,58 @@ spec:
   replicas: 3 # {"$openapi":"replicas"}
 `},
 
+	//
+	// Test Case
+	//
+	{
+		description: `Ensure comments are updated`,
+		origin: `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nginx-deployment
+  annotations:
+    config.kubernetes.io/index: '0'
+    config.kubernetes.io/merge-source: 'dest'
+    config.kubernetes.io/path: 'temp.yaml'
+spec:
+  replicas: 3 # {"$openapi":"replicas"}`,
+		update: `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nginx-deployment
+  annotations:
+    config.kubernetes.io/index: '0'
+    config.kubernetes.io/merge-source: 'updated'
+    config.kubernetes.io/path: 'temp.yaml'
+spec:
+  replicas: 3 # {"$openapi":"replicas_new"}`,
+		local: `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nginx-deployment
+  annotations:
+    config.kubernetes.io/index: '0'
+    config.kubernetes.io/merge-source: 'original'
+    config.kubernetes.io/path: 'temp.yaml'
+spec:
+  replicas: 3 # {"$openapi":"replicas"}
+`,
+		expected: `
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: nginx-deployment
+  annotations:
+    config.kubernetes.io/index: '0'
+    config.kubernetes.io/merge-source: 'updated'
+    config.kubernetes.io/path: 'temp.yaml'
+spec:
+  replicas: 3 # {"$openapi":"replicas_new"}
+`},
+
 	{
 		description: `Ensure deleted comments are not updated`,
 		origin: `

kyaml/yaml/merge3/visitor.go

@@ -4,7 +4,6 @@
 package merge3
 
 import (
-	"sigs.k8s.io/kustomize/kyaml/errors"
 	"sigs.k8s.io/kustomize/kyaml/openapi"
 	"sigs.k8s.io/kustomize/kyaml/yaml"
 	"sigs.k8s.io/kustomize/kyaml/yaml/walk"
@@ -34,6 +33,7 @@ func (m Visitor) VisitMap(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml.
 		// initialize a new value that can be recursively merged
 		return yaml.NewRNode(&yaml.Node{Kind: yaml.MappingNode}), nil
 	}
+
 	// recursively merge the dest with the original and updated
 	return nodes.Dest(), nil
 }
@@ -67,17 +67,13 @@ func (m Visitor) VisitScalar(nodes walk.Sources, s *openapi.ResourceSchema) (*ya
 		return nodes.Dest(), nil
 	}
 
-	updatedStr, err := nodes.Updated().String()
+	values, err := m.getStrValues(nodes)
 	if err != nil {
-		return nil, errors.Wrap(err)
+		return nil, err
-	}
-	originStr, err := nodes.Origin().String()
-	if err != nil {
-		return nil, errors.Wrap(err)
 	}
 
-	if updatedStr != originStr {
+	if (values.Dest == "" || values.Dest == values.Origin) && values.Origin != values.Update {
-		// change in update node
+		// if local is nil or is unchanged but there is new update
 		return nodes.Updated(), nil
 	}
 

kyaml/yaml/types.go

@@ -139,6 +139,13 @@ type ResourceIdentifier struct {
 	Kind string `yaml:"kind,omitempty"`
 }
 
+// Comments struct is comment yaml comment types
+type Comments struct {
+	LineComment string `yaml:"lineComment,omitempty"`
+	HeadComment string `yaml:"headComment,omitempty"`
+	FootComment string `yaml:"footComment,omitempty"`
+}
+
 func (r *ResourceIdentifier) GetName() string {
 	return r.Name
 }

kyaml/yaml/walk/map.go

@@ -27,11 +27,12 @@ func (l Walker) walkMap() (*yaml.RNode, error) {
 
 	// recursively set the field values on the map
 	for _, key := range l.fieldNames() {
+		var res *yaml.RNode
+		var keys []*yaml.RNode
 		if l.VisitKeysAsScalars {
 			// visit the map keys as if they were scalars,
 			// this is necessary if doing things such as copying
 			// comments
-			var keys []*yaml.RNode
 			for i := range l.Sources {
 				// construct the sources from the keys
 				if l.Sources[i] == nil {
@@ -47,7 +48,7 @@ func (l Walker) walkMap() (*yaml.RNode, error) {
 			}
 			// visit the sources as a scalar
 			// keys don't have any schema --pass in nil
-			_, err := l.Visitor.VisitScalar(keys, nil)
+			res, err = l.Visitor.VisitScalar(keys, nil)
 			if err != nil {
 				return nil, err
 			}
@@ -72,8 +73,23 @@ func (l Walker) walkMap() (*yaml.RNode, error) {
 			return nil, err
 		}
 
+		// transfer the comments of res to dest node
+		var comments yaml.Comments
+		if !yaml.IsMissingOrNull(res) {
+			comments = yaml.Comments{
+				LineComment: res.YNode().LineComment,
+				HeadComment: res.YNode().HeadComment,
+				FootComment: res.YNode().FootComment,
+			}
+			if len(keys) > 0 && !yaml.IsMissingOrNull(keys[DestIndex]) {
+				keys[DestIndex].YNode().HeadComment = res.YNode().HeadComment
+				keys[DestIndex].YNode().LineComment = res.YNode().LineComment
+				keys[DestIndex].YNode().FootComment = res.YNode().FootComment
+			}
+		}
+
 		// this handles empty and non-empty values
-		_, err = dest.Pipe(yaml.FieldSetter{Name: key, Value: val})
+		_, err = dest.Pipe(yaml.FieldSetter{Name: key, Comments: comments, Value: val})
 		if err != nil {
 			return nil, err
 		}