Copy comments from function inputs to function outputs.

kyaml/comments/comments.go

@@ -37,6 +37,18 @@ func (c *copier) VisitScalar(s walk.Sources, _ *openapi.ResourceSchema) (*yaml.R
 func (c *copier) VisitList(s walk.Sources, _ *openapi.ResourceSchema, _ walk.ListKind) (
 	*yaml.RNode, error) {
 	copy(s.Dest(), s.Origin())
+	destItems := s.Dest().Content()
+	originItems := s.Origin().Content()
+
+	for i := 0; i < len(destItems) && i < len(originItems); i++ {
+		dest := destItems[i]
+		origin := originItems[i]
+
+		if dest.Value == origin.Value {
+			copy(yaml.NewRNode(dest), yaml.NewRNode(origin))
+		}
+	}
+
 	return s.Dest(), nil
 }
 
@@ -45,13 +57,13 @@ func copy(from, to *yaml.RNode) {
 	if from == nil || to == nil {
 		return
 	}
-	if from.Document().LineComment != "" {
+	if to.Document().LineComment == "" {
 		to.Document().LineComment = from.Document().LineComment
 	}
-	if from.Document().HeadComment != "" {
+	if to.Document().HeadComment == "" {
 		to.Document().HeadComment = from.Document().HeadComment
 	}
-	if from.Document().FootComment != "" {
+	if to.Document().FootComment == "" {
 		to.Document().FootComment = from.Document().FootComment
 	}
 }

kyaml/comments/comments_test.go

@@ -4,6 +4,7 @@
 package comments
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -11,7 +12,15 @@ import (
 )
 
 func TestCopyComments(t *testing.T) {
-	from, err := yaml.Parse(`# A
+	testCases := []struct {
+		name     string
+		from     string
+		to       string
+		expected string
+	}{
+		{
+			name: "copy_comments",
+			from: `# A
 #
 # B
 
@@ -22,16 +31,274 @@ spec: # comment 1
   # comment 2
   replicas: 3 # comment 3
   # comment 4
-`)
+`,
+			to: `apiVersion: apps/v1
+kind: Deployment
+spec:
+  replicas: 4
+`,
+			expected: `# A
+#
+# B
+
+# C
+apiVersion: apps/v1
+kind: Deployment
+spec: # comment 1
+  # comment 2
+  replicas: 4 # comment 3
+  # comment 4
+`,
+		},
+
+		{
+			name: "associative_list",
+			from: `
+apiVersion: apps/v1
+kind: Deployment
+spec:
+  template:
+    spec:
+      containers:
+      - name: foo
+        image: bar # comment 1
+`,
+			to: `
+apiVersion: apps/v1
+kind: Deployment
+spec:
+  template:
+    spec:
+      containers:
+      - name: foo
+        image: bar
+`,
+			expected: `
+apiVersion: apps/v1
+kind: Deployment
+spec:
+  template:
+    spec:
+      containers:
+      - name: foo
+        image: bar # comment 1
+`,
+		},
+
+		{
+			name: "keep_comments",
+			from: `# A
+#
+# B
+
+# C
+apiVersion: apps/v1
+kind: Deployment
+spec: # comment 1
+  # comment 2
+  replicas: 3 # comment 3
+  # comment 4
+`,
+			to: `apiVersion: apps/v1
+kind: Deployment
+spec:
+  replicas: 4 # comment 5
+`,
+			expected: `# A
+#
+# B
+
+# C
+apiVersion: apps/v1
+kind: Deployment
+spec: # comment 1
+  # comment 2
+  replicas: 4 # comment 5
+  # comment 4
+`,
+		},
+
+		{
+			name: "copy_item_comments",
+			from: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a # comment
+`,
+			to: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a
+`,
+			expected: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a # comment
+`,
+		},
+
+		{
+			name: "copy_item_comments_2",
+			from: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+# comment
+- a
+`,
+			to: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a
+`,
+			expected: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+# comment
+- a
+`,
+		},
+
+		{
+			name: "copy_item_comments_middle",
+			from: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a
+- b # comment
+- c
+`,
+			to: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- d
+- b
+- e
+`,
+			expected: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- d
+- b # comment
+- e
+`,
+		},
+
+		{
+			name: "copy_item_comments_moved",
+			from: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a
+- b # comment
+- c
+`,
+			to: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a
+- c
+- b
+`,
+			expected: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a
+- c
+- b
+`,
+		},
+
+		{
+			name: "copy_item_comments_no_match",
+			from: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a # comment
+`,
+			to: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- b
+`,
+			expected: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- b
+`,
+		},
+
+		{
+			name: "copy_item_comments_add",
+			from: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a # comment
+`,
+			to: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a
+- b
+`,
+			expected: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a # comment
+- b
+`,
+		},
+
+		{
+			name: "copy_item_comments_remove",
+			from: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a # comment
+- b
+`,
+			to: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a
+`,
+			expected: `
+apiVersion: apps/v1
+kind: Deployment
+items:
+- a # comment
+`,
+		},
+	}
+
+	for i := range testCases {
+		tc := testCases[i]
+		t.Run(tc.name, func(t *testing.T) {
+			from, err := yaml.Parse(tc.from)
 			if !assert.NoError(t, err) {
 				t.FailNow()
 			}
 
-	to, err := yaml.Parse(`apiVersion: apps/v1
+			to, err := yaml.Parse(tc.to)
-kind: Deployment
-spec:
-  replicas: 4
-`)
 			if !assert.NoError(t, err) {
 				t.FailNow()
 			}
@@ -46,20 +313,9 @@ spec:
 				t.FailNow()
 			}
 
-	expected := `# A
+			if !assert.Equal(t, strings.TrimSpace(tc.expected), strings.TrimSpace(actual)) {
-#
-# B
-
-# C
-apiVersion: apps/v1
-kind: Deployment
-spec: # comment 1
-  # comment 2
-  replicas: 4 # comment 3
-  # comment 4
-`
-
-	if !assert.Equal(t, expected, actual) {
 				t.FailNow()
 			}
+		})
+	}
 }

kyaml/fn/runtime/runtimeutil/runtimeutil.go

@@ -5,11 +5,13 @@ package runtimeutil
 
 import (
 	"bytes"
+	"fmt"
 	"io"
 	"io/ioutil"
 	"path"
 	"strings"
 
+	"sigs.k8s.io/kustomize/kyaml/comments"
 	"sigs.k8s.io/kustomize/kyaml/errors"
 	"sigs.k8s.io/kustomize/kyaml/kio"
 	"sigs.k8s.io/kustomize/kyaml/kio/kioutil"
@@ -44,6 +46,8 @@ type FunctionFilter struct {
 
 	// exit saves the error returned from Run
 	exit error
+
+	ids map[string]*yaml.RNode
 }
 
 // GetExit returns the error from Run
@@ -138,6 +142,11 @@ func (c *FunctionFilter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
 		return nil, err
 	}
 
+	// set ids on each input so it is possible to copy comments from inputs back to outputs
+	if err := c.setIds(input); err != nil {
+		return nil, err
+	}
+
 	// write the input
 	err = kio.ByteWriter{
 		WrappingAPIVersion:    kio.ResourceListAPIVersion,
@@ -160,6 +169,11 @@ func (c *FunctionFilter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
 		return nil, err
 	}
 
+	// copy the comments from the inputs to the outputs
+	if err := c.setComments(output); err != nil {
+		return nil, err
+	}
+
 	if err := c.doResults(r); err != nil {
 		return nil, err
 	}
@@ -178,6 +192,50 @@ func (c *FunctionFilter) Filter(nodes []*yaml.RNode) ([]*yaml.RNode, error) {
 	return append(output, saved...), nil
 }
 
+const idAnnotation = "config.k8s.io/id"
+
+func (c *FunctionFilter) setIds(nodes []*yaml.RNode) error {
+	// set the id on each node to map inputs to outputs
+	var id int
+	c.ids = map[string]*yaml.RNode{}
+	for i := range nodes {
+		id++
+		idStr := fmt.Sprintf("%v", id)
+		err := nodes[i].PipeE(yaml.SetAnnotation(idAnnotation, idStr))
+		if err != nil {
+			return errors.Wrap(err)
+		}
+		c.ids[idStr] = nodes[i]
+	}
+	return nil
+}
+
+func (c *FunctionFilter) setComments(nodes []*yaml.RNode) error {
+	for i := range nodes {
+		node := nodes[i]
+		anID, err := node.Pipe(yaml.GetAnnotation(idAnnotation))
+		if err != nil {
+			return errors.Wrap(err)
+		}
+		if anID == nil {
+			continue
+		}
+
+		var in *yaml.RNode
+		var found bool
+		if in, found = c.ids[anID.YNode().Value]; !found {
+			continue
+		}
+		if err := comments.CopyComments(in, node); err != nil {
+			return errors.Wrap(err)
+		}
+		if err := node.PipeE(yaml.ClearAnnotation(idAnnotation)); err != nil {
+			return errors.Wrap(err)
+		}
+	}
+	return nil
+}
+
 func (c *FunctionFilter) doResults(r *kio.ByteReader) error {
 	// Write the results to a file if configured to do so
 	if c.ResultsFile != "" && r.Results != nil {

kyaml/fn/runtime/runtimeutil/runtimeutil_test.go

@@ -486,6 +486,7 @@ items:
     name: service-foo
     annotations:
       config.kubernetes.io/path: 'foo/bar/s.yaml'
+      config.k8s.io/id: '1'
 functionConfig:
   apiVersion: example.com/v1
   kind: Example
@@ -504,6 +505,7 @@ items:
     annotations:
       config.kubernetes.io/path: 'foo/bar/s.yaml'
       new: annotation
+      config.k8s.io/id: '1'
 functionConfig:
   apiVersion: example.com/v1
   kind: Example
@@ -573,6 +575,7 @@ items:
     name: service-foo
     annotations:
       config.kubernetes.io/path: 'foo/bar/s.yaml'
+      config.k8s.io/id: '1'
 functionConfig:
   apiVersion: example.com/v1
   kind: Example
@@ -591,6 +594,7 @@ items:
     annotations:
       config.kubernetes.io/path: 'foo/bar/s.yaml'
       new: annotation
+      config.k8s.io/id: '1'
 functionConfig:
   apiVersion: example.com/v1
   kind: Example
@@ -657,12 +661,14 @@ items:
     name: deployment-foo
     annotations:
       config.kubernetes.io/path: 'baz/bar/d.yaml'
+      config.k8s.io/id: '1'
 - apiVersion: v1
   kind: Service
   metadata:
     name: service-foo
     annotations:
       config.kubernetes.io/path: 'foo/bar/s.yaml'
+      config.k8s.io/id: '2'
 functionConfig:
   apiVersion: example.com/v1
   kind: Example
@@ -680,6 +686,7 @@ items:
     name: deployment-foo
     annotations:
       config.kubernetes.io/path: 'baz/bar/d.yaml'
+      config.k8s.io/id: '1'
 - apiVersion: v1
   kind: Service
   metadata:
@@ -687,6 +694,7 @@ items:
     annotations:
       config.kubernetes.io/path: 'foo/bar/s.yaml'
       new: annotation
+      config.k8s.io/id: '2'
 functionConfig:
   apiVersion: example.com/v1
   kind: Example
@@ -823,6 +831,7 @@ items:
     name: service-foo
     annotations:
       config.kubernetes.io/path: 'foo/bar/s.yaml'
+      config.k8s.io/id: '1'
 functionConfig:
   apiVersion: example.com/v1
   kind: Example
@@ -840,6 +849,7 @@ items:
     name: service-foo
     annotations:
       config.kubernetes.io/path: 'foo/bar/s.yaml'
+      config.k8s.io/id: '1'
       new: annotation
 functionConfig:
   apiVersion: example.com/v1
@@ -893,6 +903,107 @@ metadata:
   name: deployment-foo
   annotations:
     config.kubernetes.io/path: 'baz/bar/d.yaml'
+`,
+			},
+		},
+
+		{
+			name: "copy_comments",
+			run: testRun{
+				expectedInput: `apiVersion: config.kubernetes.io/v1alpha1
+kind: ResourceList
+items:
+- apiVersion: apps/v1
+  kind: Deployment
+  metadata:
+    name: deployment-foo
+    annotations:
+      config.kubernetes.io/path: 'foo/b.yaml'
+      config.k8s.io/id: '1'
+- apiVersion: v1
+  kind: Service
+  metadata:
+    name: service-foo # name comment
+    annotations:
+      config.kubernetes.io/path: 'foo/a.yaml'
+      config.k8s.io/id: '2'
+functionConfig:
+  apiVersion: example.com/v1
+  kind: Example
+  metadata:
+    name: foo
+    annotations:
+      config.kubernetes.io/path: 'foo/f.yaml'
+`,
+				// delete the comment
+				output: `apiVersion: config.kubernetes.io/v1alpha1
+kind: ResourceList
+items:
+- apiVersion: apps/v1
+  kind: Deployment
+  metadata:
+    name: deployment-foo
+    annotations:
+      config.kubernetes.io/path: 'foo/b.yaml'
+      config.k8s.io/id: '1'
+- apiVersion: v1
+  kind: Service
+  metadata:
+    name: service-foo
+    annotations:
+      config.kubernetes.io/path: 'foo/a.yaml'
+      config.k8s.io/id: '2'
+      new: annotation
+functionConfig:
+  apiVersion: example.com/v1
+  kind: Example
+  metadata:
+    name: foo
+    annotations:
+      config.kubernetes.io/path: 'foo/f.yaml'
+`,
+			},
+			functionConfig: `
+apiVersion: example.com/v1
+kind: Example
+metadata:
+  name: foo
+  annotations:
+    config.kubernetes.io/path: 'foo/f.yaml'
+`,
+			input: []string{
+				`
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: deployment-foo
+  annotations:
+    config.kubernetes.io/path: 'foo/b.yaml'
+`,
+				`
+apiVersion: v1
+kind: Service
+metadata:
+  name: service-foo # name comment
+  annotations:
+    config.kubernetes.io/path: 'foo/a.yaml'
+`},
+			expectedOutput: []string{
+				`
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: deployment-foo
+  annotations:
+    config.kubernetes.io/path: 'foo/b.yaml'
+`, `
+apiVersion: v1
+kind: Service
+metadata:
+  name: service-foo # name comment
+  annotations:
+    config.kubernetes.io/path: 'foo/a.yaml'
+    new: annotation
 `,
 			},
 		},

kyaml/fn/runtime/starlark/starlark.go

@@ -12,7 +12,6 @@ import (
 
 	"github.com/qri-io/starlib/util"
 	"go.starlark.net/starlark"
-	"sigs.k8s.io/kustomize/kyaml/comments"
 	"sigs.k8s.io/kustomize/kyaml/errors"
 	"sigs.k8s.io/kustomize/kyaml/fn/runtime/runtimeutil"
 	"sigs.k8s.io/kustomize/kyaml/kio/filters"
@@ -33,8 +32,6 @@ type Filter struct {
 	Path string
 
 	runtimeutil.FunctionFilter
-
-	ids map[string]*yaml.RNode
 }
 
 func (sf *Filter) String() string {
@@ -128,23 +125,6 @@ func (sf *Filter) readResourceList(reader io.Reader) (starlark.Value, error) {
 		return nil, errors.Wrap(err)
 	}
 
-	// set the id on each node to map inputs to outputs
-	var id int
-	sf.ids = map[string]*yaml.RNode{}
-	items, err := rn.Pipe(yaml.Lookup("items"))
-	if err != nil {
-		return nil, errors.Wrap(err)
-	}
-	err = items.VisitElements(func(node *yaml.RNode) error {
-		id++
-		idStr := fmt.Sprintf("%v", id)
-		sf.ids[idStr] = node
-		return node.PipeE(yaml.SetAnnotation("config.k8s.io/id", idStr))
-	})
-	if err != nil {
-		return nil, errors.Wrap(err)
-	}
-
 	// convert to a starlark value
 	b, err := yaml.Marshal(rn.Document()) // convert to bytes
 	if err != nil {
@@ -182,33 +162,10 @@ func (sf *Filter) writeResourceList(value starlark.Value, writer io.Writer) erro
 		return errors.Wrap(err)
 	}
 	err = items.VisitElements(func(node *yaml.RNode) error {
-		anID, err := node.Pipe(yaml.GetAnnotation("config.k8s.io/id"))
-		if err != nil {
-			return errors.Wrap(err)
-		}
-		if anID == nil {
-			return nil
-		}
-
-		var in *yaml.RNode
-		var found bool
-		if in, found = sf.ids[anID.YNode().Value]; !found {
-			return nil
-		}
-		if err := node.PipeE(yaml.ClearAnnotation("config.k8s.io/id")); err != nil {
-			return errors.Wrap(err)
-		}
-		if err := comments.CopyComments(in, node); err != nil {
-			return errors.Wrap(err)
-		}
-
 		// starlark will serialize the resources sorting the fields alphabetically,
 		// format them to have a better ordering
-		fmtFltr := filters.FormatFilter{}
+		_, err := filters.FormatFilter{}.Filter([]*yaml.RNode{node})
-		if _, err := fmtFltr.Filter([]*yaml.RNode{node}); err != nil {
+		return err
-			return errors.Wrap(err)
-		}
-		return nil
 	})
 	if err != nil {
 		return errors.Wrap(err)