From d7aeb3fc0add4cf0cd8c134d178a384e2778f15b Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 24 Feb 2023 12:44:50 +0100 Subject: [PATCH] diff: prettify premature diff log Signed-off-by: Hidde Beydals --- go.mod | 2 +- internal/cmp/simple_unstructured.go | 110 +++++++++ internal/cmp/simple_unstructured_test.go | 285 +++++++++++++++++++++++ internal/diff/differ.go | 20 +- 4 files changed, 408 insertions(+), 9 deletions(-) create mode 100644 internal/cmp/simple_unstructured.go create mode 100644 internal/cmp/simple_unstructured_test.go diff --git a/go.mod b/go.mod index 597625d3b..941cf7017 100644 --- a/go.mod +++ b/go.mod @@ -14,6 +14,7 @@ require ( github.com/fluxcd/pkg/ssa v0.24.1 github.com/fluxcd/source-controller/api v0.35.1 github.com/go-logr/logr v1.2.3 + github.com/google/go-cmp v0.5.9 github.com/hashicorp/go-retryablehttp v0.7.2 github.com/onsi/gomega v1.26.0 github.com/spf13/pflag v1.0.5 @@ -80,7 +81,6 @@ require ( github.com/golang/protobuf v1.5.2 // indirect github.com/google/btree v1.1.2 // indirect github.com/google/gnostic v0.6.9 // indirect - github.com/google/go-cmp v0.5.9 // indirect github.com/google/gofuzz v1.2.0 // indirect github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 // indirect github.com/google/uuid v1.3.0 // indirect diff --git a/internal/cmp/simple_unstructured.go b/internal/cmp/simple_unstructured.go new file mode 100644 index 000000000..6ccccb242 --- /dev/null +++ b/internal/cmp/simple_unstructured.go @@ -0,0 +1,110 @@ +/* +Copyright 2023 The Flux authors + +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 cmp + +import ( + "fmt" + "reflect" + "strings" + + "github.com/google/go-cmp/cmp" +) + +// SimpleUnstructuredReporter is a simple reporter for Unstructured objects +// that only records differences detected during comparison, in a diff-like +// format. +type SimpleUnstructuredReporter struct { + path cmp.Path + diffs []string +} + +// Report writes a diff entry if rs is not equal. In the format of: +// +// spec.replicas +// -3 +// +1 +// +// spec.template.spec.containers.[0].command.[6] +// ---deleted=true +// +// spec.template.spec.containers.[0].env.[?->1]: +// +map[name:ADDED] +func (r *SimpleUnstructuredReporter) Report(rs cmp.Result) { + if !rs.Equal() { + vx, vy := r.path.Last().Values() + isNonEmptyX := isValidAndNonEmpty(vx) + isNonEmptyY := isValidAndNonEmpty(vy) + + if !isNonEmptyX && !isNonEmptyY { + // Skip empty values. + return + } + + var sb strings.Builder + sb.WriteString(pathToString(r.path)) + sb.WriteString("\n") + if isNonEmptyX { + sb.WriteString(fmt.Sprintf("-%v", vx)) + sb.WriteString("\n") + } + if isNonEmptyY { + sb.WriteString(fmt.Sprintf("+%v", vy)) + sb.WriteString("\n") + } + r.diffs = append(r.diffs, sb.String()) + } +} + +// String returns the diff entries joined together with newline, trimmed from +// spaces. +func (r *SimpleUnstructuredReporter) String() string { + return strings.TrimSpace(strings.Join(r.diffs, "\n")) +} + +func (r *SimpleUnstructuredReporter) PushStep(ps cmp.PathStep) { + r.path = append(r.path, ps) +} + +func (r *SimpleUnstructuredReporter) PopStep() { + r.path = r.path[:len(r.path)-1] +} + +func isValidAndNonEmpty(v reflect.Value) bool { + if !v.IsValid() { + return false + } + switch v.Kind() { + case reflect.Interface: + return isValidAndNonEmpty(v.Elem()) + case reflect.Array, reflect.Chan, reflect.Map, reflect.Slice, reflect.String: + return v.Len() > 0 + } + return true +} + +func pathToString(path cmp.Path) (p string) { + for _, step := range path { + switch t := step.(type) { + case cmp.MapIndex: + p += fmt.Sprintf(".%v", t.Key()) + case cmp.SliceIndex: + p += fmt.Sprintf("%v", t.String()) + } + } + p = strings.TrimPrefix(p, ".") + return +} diff --git a/internal/cmp/simple_unstructured_test.go b/internal/cmp/simple_unstructured_test.go new file mode 100644 index 000000000..d733e5e83 --- /dev/null +++ b/internal/cmp/simple_unstructured_test.go @@ -0,0 +1,285 @@ +/* +Copyright 2023 The Flux authors + +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 cmp + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/util/yaml" +) + +func TestSimpleYAMLReporter_Report(t *testing.T) { + tests := []struct { + name string + x string + y string + want string + }{ + { + name: "added simple value", + x: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: {}`, + y: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + replicas: 1`, + want: `spec.replicas ++1`, + }, + { + name: "removed simple value", + x: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + replicas: 1`, + y: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: {}`, + want: `spec.replicas +-1`, + }, + { + name: "changed simple value", + x: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + replicas: 1`, + y: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + replicas: 2`, + want: `spec.replicas +-1 ++2`, + }, + { + name: "added map with value", + x: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: {}`, + y: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: + containers: + - name: container`, + want: `spec.template.spec.containers ++[map[name:container]]`, + }, + { + name: "removed map with value", + x: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: + containers: + - name: container`, + y: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: {}`, + want: `spec.template.spec.containers +-[map[name:container]]`, + }, + { + name: "changed map with value", + x: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: + containers: + - name: container`, + y: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: + containers: + - name: container2`, + want: `spec.template.spec.containers[0].name +-container ++container2`, + }, + { + name: "added list item value", + x: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: + containers: + - name: container + env: []`, + y: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: + containers: + - name: container + env: + - name: env`, + want: `spec.template.spec.containers[0].env[?->0] ++map[name:env]`, + }, + { + name: "removed list item value", + x: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: + containers: + - name: container + env: + - name: env`, + y: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: + containers: + - name: container + env: []`, + want: `spec.template.spec.containers[0].env[0->?] +-map[name:env]`, + }, + { + name: "changed list item value", + x: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: + containers: + - name: container + env: + - name: env`, + y: `apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment +spec: + template: + spec: + containers: + - name: container + env: + - name: env2`, + want: `spec.template.spec.containers[0].env[0].name +-env ++env2`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + x, err := yamlToUnstructured(tt.x) + if err != nil { + t.Fatal("failed to parse x", err) + } + y, err := yamlToUnstructured(tt.y) + if err != nil { + t.Fatal("failed to parse y", err) + } + + r := SimpleUnstructuredReporter{} + _ = cmp.Diff(x, y, cmp.Reporter(&r)) + result := r.String() + g.Expect(result).To(Equal(tt.want), result) + }) + } +} + +func TestSimpleYAMLReporter_String(t *testing.T) { + tests := []struct { + name string + diffs []string + want string + }{ + {name: "trims space", diffs: []string{" at start", "in\nbetween", "at end\n"}, want: `at start +in +between +at end`}, + {name: "joins with newline", diffs: []string{"a", "b", "c"}, want: `a +b +c`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &SimpleUnstructuredReporter{ + diffs: tt.diffs, + } + if got := r.String(); got != tt.want { + t.Errorf("String() = %v, want %v", got, tt.want) + } + }) + } +} + +func yamlToUnstructured(str string) (*unstructured.Unstructured, error) { + var obj map[string]interface{} + if err := yaml.Unmarshal([]byte(str), &obj); err != nil { + return nil, err + } + return &unstructured.Unstructured{Object: obj}, nil +} diff --git a/internal/diff/differ.go b/internal/diff/differ.go index 1b0aa27d5..dbd9f3f78 100644 --- a/internal/diff/differ.go +++ b/internal/diff/differ.go @@ -19,19 +19,20 @@ package diff import ( "context" "fmt" - "github.com/fluxcd/pkg/runtime/logger" - "github.com/google/go-cmp/cmp" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - ctrl "sigs.k8s.io/controller-runtime" "strings" + "github.com/fluxcd/pkg/runtime/client" + "github.com/fluxcd/pkg/ssa" + "github.com/google/go-cmp/cmp" "helm.sh/helm/v3/pkg/release" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/util/errors" + ctrl "sigs.k8s.io/controller-runtime" - "github.com/fluxcd/pkg/runtime/client" - "github.com/fluxcd/pkg/ssa" + "github.com/fluxcd/pkg/runtime/logger" helmv1 "github.com/fluxcd/helm-controller/api/v2beta1" + intcmp "github.com/fluxcd/helm-controller/internal/cmp" "github.com/fluxcd/helm-controller/internal/util" ) @@ -126,10 +127,13 @@ func (d *Differ) Diff(ctx context.Context, rel *release.Release) (*ssa.ChangeSet if entry.Action == ssa.ConfiguredAction { // TODO: remove this once we have a better way to log the diff // for example using a custom dyff reporter, or a flux CLI command - ctrl.LoggerFrom(ctx).V(logger.DebugLevel).Info(entry.Subject + " diff:" + cmp.Diff( + r := intcmp.SimpleUnstructuredReporter{} + if diff := cmp.Diff( unstructuredWithoutStatus(releaseObject).UnstructuredContent(), unstructuredWithoutStatus(clusterObject).UnstructuredContent(), - )) + cmp.Reporter(&r)); diff != "" { + ctrl.LoggerFrom(ctx).V(logger.DebugLevel).Info(entry.Subject + " diff:\n" + r.String()) + } } case ssa.SkippedAction: changeSet.Add(*entry)