From f2b143889b44170c8f099c6821863654da30f0e3 Mon Sep 17 00:00:00 2001 From: Katrina Verey Date: Thu, 14 Jul 2022 13:46:41 -0400 Subject: [PATCH] Code review feedback --- api/filters/filtersutil/setters.go | 3 +-- api/filters/nameref/nameref.go | 2 +- api/filters/namespace/namespace.go | 13 ++++--------- api/internal/builtins/NamespaceTransformer.go | 8 ++++---- .../namespacetransformer/NamespaceTransformer.go | 8 ++++---- .../NamespaceTransformer_test.go | 4 ++-- 6 files changed, 16 insertions(+), 22 deletions(-) diff --git a/api/filters/filtersutil/setters.go b/api/filters/filtersutil/setters.go index 2332662c157..f7766678566 100644 --- a/api/filters/filtersutil/setters.go +++ b/api/filters/filtersutil/setters.go @@ -94,8 +94,7 @@ func hasExistingValue(node *yaml.RNode, key string) bool { if node.IsNilOrEmpty() { return false } - if key == "" { - // scalar node + if err := yaml.ErrorIfInvalid(node, yaml.ScalarNode); err == nil { return yaml.GetValue(node) != "" } entry := node.Field(key) diff --git a/api/filters/nameref/nameref.go b/api/filters/nameref/nameref.go index 65f3174b1ca..7549ab70011 100644 --- a/api/filters/nameref/nameref.go +++ b/api/filters/nameref/nameref.go @@ -337,7 +337,7 @@ func (f Filter) selectReferral( return candidates[0], nil } ids := getIds(candidates) - return nil, fmt.Errorf(" found multiple possible referrals: %s\n%s", ids, f.failureDetails(candidates)) + return nil, fmt.Errorf("found multiple possible referrals: %s\n%s", ids, f.failureDetails(candidates)) } func (f Filter) failureDetails(resources []*resource.Resource) string { diff --git a/api/filters/namespace/namespace.go b/api/filters/namespace/namespace.go index ff65fcaba48..208a1925005 100644 --- a/api/filters/namespace/namespace.go +++ b/api/filters/namespace/namespace.go @@ -19,8 +19,8 @@ type Filter struct { // FsSlice contains the FieldSpecs to locate the namespace field FsSlice types.FsSlice `json:"fieldSpecs,omitempty" yaml:"fieldSpecs,omitempty"` - // SkipExisting means only blank namespace fields will be set - SkipExisting bool `json:"skipExisting" yaml:"skipExisting"` + // UnsetOnly means only blank namespace fields will be set + UnsetOnly bool `json:"UnsetOnly" yaml:"UnsetOnly"` trackableSetter filtersutil.TrackableSetter } @@ -134,12 +134,7 @@ func (ns Filter) roleBindingHack(obj *yaml.RNode, gvk resid.Gvk) error { } func isRoleBinding(kind string) bool { - switch kind { - case roleBindingKind, clusterRoleBindingKind: - return true - default: - return false - } + return kind == roleBindingKind || kind == clusterRoleBindingKind } // removeRoleBindingFieldSpecs removes from the list fieldspecs that @@ -167,7 +162,7 @@ func (ns Filter) removeMetaNamespaceFieldSpecs(fs types.FsSlice) types.FsSlice { } func (ns *Filter) fieldSetter() filtersutil.SetFn { - if ns.SkipExisting { + if ns.UnsetOnly { return ns.trackableSetter.SetEntryIfEmpty("", ns.Namespace, yaml.NodeTagString) } return ns.trackableSetter.SetEntry("", ns.Namespace, yaml.NodeTagString) diff --git a/api/internal/builtins/NamespaceTransformer.go b/api/internal/builtins/NamespaceTransformer.go index 4f3846e9e51..03a664cff88 100644 --- a/api/internal/builtins/NamespaceTransformer.go +++ b/api/internal/builtins/NamespaceTransformer.go @@ -16,7 +16,7 @@ import ( type NamespaceTransformerPlugin struct { types.ObjectMeta `json:"metadata,omitempty" yaml:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` FieldSpecs []types.FieldSpec `json:"fieldSpecs,omitempty" yaml:"fieldSpecs,omitempty"` - SkipExisting bool `json:"skipExisting" yaml:"skipExisting"` + UnsetOnly bool `json:"unsetOnly" yaml:"unsetOnly"` } func (p *NamespaceTransformerPlugin) Config( @@ -37,9 +37,9 @@ func (p *NamespaceTransformerPlugin) Transform(m resmap.ResMap) error { } r.StorePreviousId() if err := r.ApplyFilter(namespace.Filter{ - Namespace: p.Namespace, - FsSlice: p.FieldSpecs, - SkipExisting: p.SkipExisting, + Namespace: p.Namespace, + FsSlice: p.FieldSpecs, + UnsetOnly: p.UnsetOnly, }); err != nil { return err } diff --git a/plugin/builtin/namespacetransformer/NamespaceTransformer.go b/plugin/builtin/namespacetransformer/NamespaceTransformer.go index 1fc1c58d42c..249fdf1ea4b 100644 --- a/plugin/builtin/namespacetransformer/NamespaceTransformer.go +++ b/plugin/builtin/namespacetransformer/NamespaceTransformer.go @@ -17,7 +17,7 @@ import ( type plugin struct { types.ObjectMeta `json:"metadata,omitempty" yaml:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` FieldSpecs []types.FieldSpec `json:"fieldSpecs,omitempty" yaml:"fieldSpecs,omitempty"` - SkipExisting bool `json:"skipExisting" yaml:"skipExisting"` + UnsetOnly bool `json:"unsetOnly" yaml:"unsetOnly"` } //noinspection GoUnusedGlobalVariable @@ -41,9 +41,9 @@ func (p *plugin) Transform(m resmap.ResMap) error { } r.StorePreviousId() if err := r.ApplyFilter(namespace.Filter{ - Namespace: p.Namespace, - FsSlice: p.FieldSpecs, - SkipExisting: p.SkipExisting, + Namespace: p.Namespace, + FsSlice: p.FieldSpecs, + UnsetOnly: p.UnsetOnly, }); err != nil { return err } diff --git a/plugin/builtin/namespacetransformer/NamespaceTransformer_test.go b/plugin/builtin/namespacetransformer/NamespaceTransformer_test.go index 0090841354a..efcc90c92a4 100644 --- a/plugin/builtin/namespacetransformer/NamespaceTransformer_test.go +++ b/plugin/builtin/namespacetransformer/NamespaceTransformer_test.go @@ -272,7 +272,7 @@ metadata: }) } -func TestNamespaceTransformer_SkipExistingTrue(t *testing.T) { +func TestNamespaceTransformer_UnsetOnlyTrue(t *testing.T) { th := kusttest_test.MakeEnhancedHarness(t). PrepBuiltin("NamespaceTransformer") defer th.Reset() @@ -282,7 +282,7 @@ kind: NamespaceTransformer metadata: name: notImportantHere namespace: test -skipExisting: true +unsetOnly: true fieldSpecs: - path: metadata/namespace create: true