From abc419b5f91a1d225c009855962662dcd831c72f Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Sat, 6 Jul 2019 08:54:42 -0500 Subject: [PATCH 1/3] Add Absorb method to VarSet and DeepEqual to Var --- pkg/target/kusttarget_test.go | 5 ++-- pkg/types/var.go | 53 +++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/pkg/target/kusttarget_test.go b/pkg/target/kusttarget_test.go index f3fb4cc7d..bce667681 100644 --- a/pkg/target/kusttarget_test.go +++ b/pkg/target/kusttarget_test.go @@ -6,7 +6,6 @@ package target_test import ( "encoding/base64" "fmt" - "reflect" "strings" "testing" @@ -334,7 +333,7 @@ vars: t.Fatalf("unexpected size %d", len(vars)) } for i := range vars[:2] { - if !reflect.DeepEqual(vars[i], someVars[i]) { + if !vars[i].DeepEqual(someVars[i]) { t.Fatalf("unexpected var[%d]:\n %v\n %v", i, vars[i], someVars[i]) } } @@ -387,7 +386,7 @@ resources: t.Fatalf("expected 4 vars, got %d", len(vars)) } for i := range vars { - if !reflect.DeepEqual(vars[i], someVars[i]) { + if !vars[i].DeepEqual(someVars[i]) { t.Fatalf("unexpected var[%d]:\n %v\n %v", i, vars[i], someVars[i]) } } diff --git a/pkg/types/var.go b/pkg/types/var.go index 44bd0f2c8..3a911f625 100644 --- a/pkg/types/var.go +++ b/pkg/types/var.go @@ -18,6 +18,7 @@ package types import ( "fmt" + "reflect" "sort" "strings" @@ -71,6 +72,19 @@ func (v *Var) defaulting() { } } +// VarEquals returns true if var a and b are Equals. +func (v Var) DeepEqual(other Var) bool { + v.ObjRef.GVK() + if v.FieldRef.FieldPath == "" { + v.FieldRef.FieldPath = "metadata.name" + } + other.ObjRef.GVK() + if other.FieldRef.FieldPath == "" { + other.FieldRef.FieldPath = "metadata.name" + } + return reflect.DeepEqual(v, other) +} + // VarSet is a set of Vars where no var.Name is repeated. type VarSet struct { set map[string]Var @@ -135,6 +149,45 @@ func (vs *VarSet) Merge(v Var) error { return nil } +// AbsorbSet absorbs other vars with error on (name,value) collision. +func (vs *VarSet) AbsorbSet(incoming VarSet) error { + for _, v := range incoming.set { + vs.Absorb(v) + } + return nil +} + +// AbsorbSlice absorbs a Var slice with error on (name,value) collision. +// Empty fields in incoming vars are defaulted. +func (vs *VarSet) AbsorbSlice(incoming []Var) error { + for _, v := range incoming { + if err := vs.Absorb(v); err != nil { + return err + } + } + return nil +} + +// Absorb absorbs another Var with error on (name,value) collision. +// Empty fields in incoming Var is defaulted. +func (vs *VarSet) Absorb(v Var) error { + conflicting := vs.Get(v.Name) + if conflicting == nil { + // no conflict. The var is valid. + v.defaulting() + vs.set[v.Name] = v + return nil + } + + if !v.DeepEqual(*conflicting) { + // two vars with the same name are pointing at two + // different resources. + return fmt.Errorf( + "var '%s' already encountered", v.Name) + } + return nil +} + // Contains is true if the set has the other var. func (vs *VarSet) Contains(other Var) bool { return vs.Get(other.Name) != nil From b7405f387213bd322404d37ead770269b4e802a7 Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Sat, 6 Jul 2019 09:18:54 -0500 Subject: [PATCH 2/3] Test new types.Var.DeepEqual method. --- pkg/target/kusttarget_test.go | 12 ++++++++++-- pkg/types/var.go | 29 +++++++++++++++-------------- pkg/types/var_test.go | 2 +- 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/pkg/target/kusttarget_test.go b/pkg/target/kusttarget_test.go index bce667681..68bc55d7f 100644 --- a/pkg/target/kusttarget_test.go +++ b/pkg/target/kusttarget_test.go @@ -6,6 +6,7 @@ package target_test import ( "encoding/base64" "fmt" + "reflect" "strings" "testing" @@ -333,7 +334,12 @@ vars: t.Fatalf("unexpected size %d", len(vars)) } for i := range vars[:2] { - if !vars[i].DeepEqual(someVars[i]) { + // We have to enforce the Defaulting call in someVars[i] + // to protect from a potential call of vars[i].ObjRef.GVK() + // during AccumulateTarget + vars[i].Defaulting() + someVars[i].Defaulting() + if !reflect.DeepEqual(vars[i], someVars[i]) { t.Fatalf("unexpected var[%d]:\n %v\n %v", i, vars[i], someVars[i]) } } @@ -386,7 +392,9 @@ resources: t.Fatalf("expected 4 vars, got %d", len(vars)) } for i := range vars { - if !vars[i].DeepEqual(someVars[i]) { + vars[i].Defaulting() + someVars[i].Defaulting() + if !reflect.DeepEqual(vars[i], someVars[i]) { t.Fatalf("unexpected var[%d]:\n %v\n %v", i, vars[i], someVars[i]) } } diff --git a/pkg/types/var.go b/pkg/types/var.go index 3a911f625..1306c1b02 100644 --- a/pkg/types/var.go +++ b/pkg/types/var.go @@ -66,22 +66,21 @@ type FieldSelector struct { } // defaulting sets reference to field used by default. -func (v *Var) defaulting() { +func (v *Var) Defaulting() { if v.FieldRef.FieldPath == "" { v.FieldRef.FieldPath = defaultFieldPath } + v.ObjRef.GVK() } -// VarEquals returns true if var a and b are Equals. +// DeepEqual returns true if var a and b are Equals. +// Note 1: The objects are unchanged by the VarEqual +// Note 2: Should be normalize be FieldPath before doing +// the DeepEqual. spec.a[b] is supposed to be the same +// as spec.a.b func (v Var) DeepEqual(other Var) bool { - v.ObjRef.GVK() - if v.FieldRef.FieldPath == "" { - v.FieldRef.FieldPath = "metadata.name" - } - other.ObjRef.GVK() - if other.FieldRef.FieldPath == "" { - other.FieldRef.FieldPath = "metadata.name" - } + v.Defaulting() + other.Defaulting() return reflect.DeepEqual(v, other) } @@ -144,7 +143,7 @@ func (vs *VarSet) Merge(v Var) error { return fmt.Errorf( "var '%s' already encountered", v.Name) } - v.defaulting() + v.Defaulting() vs.set[v.Name] = v return nil } @@ -152,7 +151,9 @@ func (vs *VarSet) Merge(v Var) error { // AbsorbSet absorbs other vars with error on (name,value) collision. func (vs *VarSet) AbsorbSet(incoming VarSet) error { for _, v := range incoming.set { - vs.Absorb(v) + if err := vs.Absorb(v); err != nil { + return err + } } return nil } @@ -174,12 +175,12 @@ func (vs *VarSet) Absorb(v Var) error { conflicting := vs.Get(v.Name) if conflicting == nil { // no conflict. The var is valid. - v.defaulting() + v.Defaulting() vs.set[v.Name] = v return nil } - if !v.DeepEqual(*conflicting) { + if !reflect.DeepEqual(v, *conflicting) { // two vars with the same name are pointing at two // different resources. return fmt.Errorf( diff --git a/pkg/types/var_test.go b/pkg/types/var_test.go index 95f799e02..efd3db7a9 100644 --- a/pkg/types/var_test.go +++ b/pkg/types/var_test.go @@ -81,7 +81,7 @@ func TestDefaulting(t *testing.T) { Name: "my-secret", }, } - v.defaulting() + v.Defaulting() if v.FieldRef.FieldPath != defaultFieldPath { t.Fatalf("expected %s, got %v", defaultFieldPath, v.FieldRef.FieldPath) From d783bbc0bca6823da1ffbec40b9cff7bf343e731 Mon Sep 17 00:00:00 2001 From: Jerome Brette Date: Thu, 25 Jul 2019 03:13:15 +0000 Subject: [PATCH 3/3] DeepEqual method seems cleaner than adding Defaulting before every reflect.DeepEqual call --- pkg/target/kusttarget_test.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/pkg/target/kusttarget_test.go b/pkg/target/kusttarget_test.go index 68bc55d7f..a66416d4d 100644 --- a/pkg/target/kusttarget_test.go +++ b/pkg/target/kusttarget_test.go @@ -6,7 +6,6 @@ package target_test import ( "encoding/base64" "fmt" - "reflect" "strings" "testing" @@ -334,12 +333,10 @@ vars: t.Fatalf("unexpected size %d", len(vars)) } for i := range vars[:2] { - // We have to enforce the Defaulting call in someVars[i] - // to protect from a potential call of vars[i].ObjRef.GVK() + // By using Var.DeepEqual, we are protecting the code + // from a potential invocation of vars[i].ObjRef.GVK() // during AccumulateTarget - vars[i].Defaulting() - someVars[i].Defaulting() - if !reflect.DeepEqual(vars[i], someVars[i]) { + if !vars[i].DeepEqual(someVars[i]) { t.Fatalf("unexpected var[%d]:\n %v\n %v", i, vars[i], someVars[i]) } } @@ -392,9 +389,10 @@ resources: t.Fatalf("expected 4 vars, got %d", len(vars)) } for i := range vars { - vars[i].Defaulting() - someVars[i].Defaulting() - if !reflect.DeepEqual(vars[i], someVars[i]) { + // By using Var.DeepEqual, we are protecting the code + // from a potential invocation of vars[i].ObjRef.GVK() + // during AccumulateTarget + if !vars[i].DeepEqual(someVars[i]) { t.Fatalf("unexpected var[%d]:\n %v\n %v", i, vars[i], someVars[i]) } }