Heal Comments on Package Resources Update (#3084)

Author: martinmaly

porch/pkg/engine/builtin_test.go

@@ -16,7 +16,6 @@ package engine
 
 import (
 	"context"
-	"flag"
 	"os"
 	"path/filepath"
 	"testing"
@@ -25,15 +24,10 @@ import (
 	"github.com/google/go-cmp/cmp"
 )
 
-var (
-	update = flag.Bool("update", false, "update golden files")
+const (
+	updateGoldenFiles = "UPDATE_GOLDEN_FILES"
 )
 
-func TestMain(m *testing.M) {
-	flag.Parse()
-	os.Exit(m.Run())
-}
-
 func TestUnknownBuiltinFunctionMutation(t *testing.T) {
 	const doesNotExist = "function-does-not-exist"
 	m, err := newBuiltinFunctionMutation(doesNotExist)
@@ -62,7 +56,7 @@ func TestPackageContext(t *testing.T) {
 
 	expectedPackage := filepath.Join(testdata, "expected")
 
-	if *update {
+	if os.Getenv(updateGoldenFiles) != "" {
 		if err := os.RemoveAll(expectedPackage); err != nil {
 			t.Fatalf("Failed to update golden files: %v", err)
 		}

porch/pkg/engine/engine.go

@@ -34,6 +34,9 @@ import (
 	"github.com/GoogleContainerTools/kpt/porch/pkg/repository"
 	"go.opentelemetry.io/otel"
 	"go.opentelemetry.io/otel/trace"
+	"sigs.k8s.io/kustomize/kyaml/comments"
+	"sigs.k8s.io/kustomize/kyaml/kio"
+	"sigs.k8s.io/kustomize/kyaml/yaml"
 )
 
 var tracer = otel.Tracer("engine")
@@ -508,7 +511,10 @@ func (m *mutationReplaceResources) Apply(ctx context.Context, resources reposito
 	patch := &api.PackagePatchTaskSpec{}
 
 	old := resources.Contents
-	new := m.newResources.Spec.Resources
+	new, err := healConfig(old, m.newResources.Spec.Resources)
+	if err != nil {
+		return repository.PackageResources{}, nil, fmt.Errorf("failed to heal resources: %w", err)
+	}
 
 	for k, newV := range new {
 		oldV, ok := old[k]
@@ -546,3 +552,48 @@ func (m *mutationReplaceResources) Apply(ctx context.Context, resources reposito
 
 	return repository.PackageResources{Contents: new}, task, nil
 }
+
+func healConfig(old, new map[string]string) (map[string]string, error) {
+	// Copy comments from old config to new
+	oldResources, err := (&packageReader{
+		input: repository.PackageResources{Contents: old},
+		extra: map[string]string{},
+	}).Read()
+	if err != nil {
+		return nil, fmt.Errorf("failed to read old packge resources: %w", err)
+	}
+
+	var filter kio.FilterFunc = func(r []*yaml.RNode) ([]*yaml.RNode, error) {
+		for _, n := range r {
+			for _, original := range oldResources {
+				if n.GetNamespace() == original.GetNamespace() &&
+					n.GetName() == original.GetName() &&
+					n.GetApiVersion() == original.GetApiVersion() &&
+					n.GetKind() == original.GetKind() {
+					comments.CopyComments(original, n)
+				}
+			}
+		}
+		return r, nil
+	}
+
+	out := &packageWriter{
+		output: repository.PackageResources{
+			Contents: map[string]string{},
+		},
+	}
+
+	if err := (kio.Pipeline{
+		Inputs: []kio.Reader{&packageReader{
+			input: repository.PackageResources{Contents: new},
+			extra: map[string]string{},
+		}},
+		Filters:               []kio.Filter{filter},
+		Outputs:               []kio.Writer{out},
+		ContinueOnEmptyResult: true,
+	}).Execute(); err != nil {
+		return nil, err
+	}
+
+	return out.output.Contents, nil
+}

porch/pkg/engine/kio.go

@@ -84,6 +84,7 @@ func (w *packageWriter) Write(nodes []*yaml.RNode) error {
 			Writer: buf,
 			ClearAnnotations: []string{
 				kioutil.PathAnnotation,
+				kioutil.LegacyPathAnnotation,
 			},
 		}
 		if err := bw.Write(nodes); err != nil {

porch/pkg/engine/replace_test.go

@@ -0,0 +1,82 @@
+// Copyright 2022 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package engine
+
+import (
+	"bytes"
+	"context"
+	"path/filepath"
+	"testing"
+
+	"github.com/GoogleContainerTools/kpt/porch/api/porch/v1alpha1"
+	"github.com/GoogleContainerTools/kpt/porch/pkg/repository"
+	"github.com/google/go-cmp/cmp"
+	"sigs.k8s.io/kustomize/kyaml/yaml"
+)
+
+func TestReplaceResources(t *testing.T) {
+	ctx := context.Background()
+
+	input := readPackage(t, filepath.Join("testdata", "replace"))
+	nocomment := removeComments(t, input)
+
+	replace := &mutationReplaceResources{
+		newResources: &v1alpha1.PackageRevisionResources{
+			Spec: v1alpha1.PackageRevisionResourcesSpec{
+				Resources: nocomment.Contents,
+			},
+		},
+		oldResources: &v1alpha1.PackageRevisionResources{
+			Spec: v1alpha1.PackageRevisionResourcesSpec{
+				Resources: input.Contents,
+			},
+		},
+	}
+
+	output, _, err := replace.Apply(ctx, input)
+	if err != nil {
+		t.Fatalf("mutationReplaceResources.Apply failed: %v", err)
+	}
+
+	if !cmp.Equal(input, output) {
+		t.Errorf("Diff: (-want,+got): %s", cmp.Diff(input, output))
+	}
+}
+
+func removeComments(t *testing.T, r repository.PackageResources) repository.PackageResources {
+	t.Helper()
+
+	out := repository.PackageResources{
+		Contents: map[string]string{},
+	}
+
+	for k, v := range r.Contents {
+		var data interface{}
+		if err := yaml.Unmarshal([]byte(v), &data); err != nil {
+			t.Fatalf("Failed to unmarshal %q: %v", k, err)
+		}
+
+		var nocomment bytes.Buffer
+		encoder := yaml.NewEncoder(&nocomment)
+		encoder.SetIndent(0)
+		if err := encoder.Encode(data); err != nil {
+			t.Fatalf("Failed to re-encode yaml output: %v", err)
+		}
+
+		out.Contents[k] = nocomment.String()
+	}
+
+	return out
+}

porch/pkg/engine/testdata/context/expected/Kptfile

@@ -2,7 +2,5 @@ apiVersion: kpt.dev/v1
 kind: Kptfile
 metadata:
   name: input
-  annotations:
-    config.kubernetes.io/path: 'Kptfile'
 info:
   description: Builtin Function Test Package

porch/pkg/engine/testdata/context/expected/bucket.yaml

@@ -5,7 +5,6 @@ metadata:
   namespace: bucket-namespace
   annotations:
     cnrm.cloud.google.com/project-id: bucket-project
-    config.kubernetes.io/path: 'bucket.yaml'
 spec:
   storageClass: standard
   uniformBucketLevelAccess: true

porch/pkg/engine/testdata/context/expected/package-context.yaml

@@ -4,6 +4,5 @@ metadata:
   name: kptfile.kpt.dev
   annotations:
     config.kubernetes.io/local-config: "true"
-    config.kubernetes.io/path: 'package-context.yaml'
 data:
   name: input

porch/pkg/engine/testdata/replace/Kptfile

@@ -0,0 +1,23 @@
+# top comment
+apiVersion: kpt.dev/v1
+# Kptfile info
+info:
+  # Kptfile description
+  description: A Google Cloud Storage bucket
+# Kptfile kind
+kind: Kptfile
+# Kptfile metadata
+metadata:
+  annotations:
+    blueprints.cloud.google.com/title: Google Cloud Storage Bucket blueprint
+  name: simple-bucket
+# Kptfile pipeline
+pipeline:
+  # Kptfile mutators
+  mutators:
+  - configMap:
+      name: updated-bucket-name
+      namespace: updated-namespace
+      project-id: updated-project-id
+      storage-class: updated-storage-class
+    image: gcr.io/kpt-fn/apply-setters:v0.2.0

porch/pkg/engine/testdata/replace/bucket.yaml

@@ -0,0 +1,18 @@
+# top comment
+apiVersion: storage.cnrm.cloud.google.com/v1beta1
+kind: StorageBucket
+# metadata comment
+metadata: # kpt-merge: config-control/blueprints-project-bucket
+  # annotations comment
+  annotations:
+    cnrm.cloud.google.com/force-destroy: "false"
+    cnrm.cloud.google.com/project-id: blueprints-project # kpt-set: ${project-id}
+  name: blueprints-project-bucket # kpt-set: ${project-id}-${name}
+  namespace: config-control # kpt-set: ${namespace}
+# spec comment
+spec:
+  storageClass: standard # kpt-set: ${storage-class}
+  uniformBucketLevelAccess: true
+  # Versioning is enabled
+  versioning:
+    enabled: false