Update setter comments correctly on updates

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

@@ -14,8 +14,9 @@ func Merge(dest, original, update *yaml.RNode) (*yaml.RNode, error) {
 	// if update == nil && original != nil => declarative deletion
 
 	return walk.Walker{
-		Visitor: Visitor{},
-		Sources: []*yaml.RNode{dest, original, update}}.Walk()
+		Visitor:            Visitor{},
+		VisitKeysAsScalars: true,
+		Sources:            []*yaml.RNode{dest, original, update}}.Walk()
 }
 
 func MergeStrings(dest, original, update string, infer bool) (string, error) {
@@ -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)
-	}
-	originStr, err := nodes.Origin().String()
-	if err != nil {
-		return nil, errors.Wrap(err)
+		return nil, err
 	}
 
-	if updatedStr != originStr {
-		// change in update node
+	if (values.Dest == "" || values.Dest == values.Origin) && values.Origin != values.Update {
+		// if local is nil or is unchanged but there is new update
 		return nodes.Updated(), nil
 	}