From c6bc4284dfea4ca93f24f5dca7d3d6c17f3a5c2a Mon Sep 17 00:00:00 2001 From: Martin Maly Date: Thu, 5 May 2022 21:09:17 -0700 Subject: [PATCH] Heal Comments on Package Resources Update (#3084) --- porch/pkg/engine/builtin_test.go | 12 +-- porch/pkg/engine/engine.go | 53 +++++++++++- porch/pkg/engine/kio.go | 1 + porch/pkg/engine/replace_test.go | 82 +++++++++++++++++++ .../engine/testdata/context/expected/Kptfile | 2 - .../testdata/context/expected/bucket.yaml | 1 - .../context/expected/package-context.yaml | 1 - porch/pkg/engine/testdata/replace/Kptfile | 23 ++++++ porch/pkg/engine/testdata/replace/bucket.yaml | 18 ++++ 9 files changed, 179 insertions(+), 14 deletions(-) create mode 100644 porch/pkg/engine/replace_test.go create mode 100644 porch/pkg/engine/testdata/replace/Kptfile create mode 100644 porch/pkg/engine/testdata/replace/bucket.yaml diff --git a/porch/pkg/engine/builtin_test.go b/porch/pkg/engine/builtin_test.go index c3ba714c63..bc4bf5091b 100644 --- a/porch/pkg/engine/builtin_test.go +++ b/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) } diff --git a/porch/pkg/engine/engine.go b/porch/pkg/engine/engine.go index f696be0826..c1a095888d 100644 --- a/porch/pkg/engine/engine.go +++ b/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 +} diff --git a/porch/pkg/engine/kio.go b/porch/pkg/engine/kio.go index b34bbe8f05..2699a3d586 100644 --- a/porch/pkg/engine/kio.go +++ b/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 { diff --git a/porch/pkg/engine/replace_test.go b/porch/pkg/engine/replace_test.go new file mode 100644 index 0000000000..40ee312d16 --- /dev/null +++ b/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 +} diff --git a/porch/pkg/engine/testdata/context/expected/Kptfile b/porch/pkg/engine/testdata/context/expected/Kptfile index 014b72d7d9..1e92a6cfe5 100644 --- a/porch/pkg/engine/testdata/context/expected/Kptfile +++ b/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 diff --git a/porch/pkg/engine/testdata/context/expected/bucket.yaml b/porch/pkg/engine/testdata/context/expected/bucket.yaml index 6dd6cf31aa..5dad26d4f8 100644 --- a/porch/pkg/engine/testdata/context/expected/bucket.yaml +++ b/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 diff --git a/porch/pkg/engine/testdata/context/expected/package-context.yaml b/porch/pkg/engine/testdata/context/expected/package-context.yaml index 7fa6502b9c..d1b0f5b9db 100644 --- a/porch/pkg/engine/testdata/context/expected/package-context.yaml +++ b/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 diff --git a/porch/pkg/engine/testdata/replace/Kptfile b/porch/pkg/engine/testdata/replace/Kptfile new file mode 100644 index 0000000000..9529df9a5f --- /dev/null +++ b/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 diff --git a/porch/pkg/engine/testdata/replace/bucket.yaml b/porch/pkg/engine/testdata/replace/bucket.yaml new file mode 100644 index 0000000000..935e6573c0 --- /dev/null +++ b/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