From e9ea157ff16049bfc83745e7e61e0abf4b0997f4 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 | 4 ++-- .../namespacetransformer/NamespaceTransformer.go | 4 ++-- .../NamespaceTransformer_test.go | 4 ++-- 6 files changed, 12 insertions(+), 18 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 8243cb938e0..1eb359aafbe 100644 --- a/api/filters/namespace/namespace.go +++ b/api/filters/namespace/namespace.go @@ -20,8 +20,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"` // SetRoleBindingSubjects determines which subject fields in RoleBinding and ClusterRoleBinding // objects will have their namespace fields set. Overrides field specs provided for these types, if any. @@ -160,12 +160,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 @@ -193,7 +188,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 7d07380d029..8f1142b196b 100644 --- a/api/internal/builtins/NamespaceTransformer.go +++ b/api/internal/builtins/NamespaceTransformer.go @@ -17,7 +17,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"` SetRoleBindingSubjects namespace.RoleBindingSubjectMode `json:"setRoleBindingSubjects" yaml:"setRoleBindingSubjects"` } @@ -54,7 +54,7 @@ func (p *NamespaceTransformerPlugin) Transform(m resmap.ResMap) error { Namespace: p.Namespace, FsSlice: p.FieldSpecs, SetRoleBindingSubjects: p.SetRoleBindingSubjects, - SkipExisting: p.SkipExisting, + UnsetOnly: p.UnsetOnly, }); err != nil { return err } diff --git a/plugin/builtin/namespacetransformer/NamespaceTransformer.go b/plugin/builtin/namespacetransformer/NamespaceTransformer.go index 67e39f5f80d..7e2835997b1 100644 --- a/plugin/builtin/namespacetransformer/NamespaceTransformer.go +++ b/plugin/builtin/namespacetransformer/NamespaceTransformer.go @@ -18,7 +18,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"` SetRoleBindingSubjects namespace.RoleBindingSubjectMode `json:"setRoleBindingSubjects" yaml:"setRoleBindingSubjects"` } @@ -58,7 +58,7 @@ func (p *plugin) Transform(m resmap.ResMap) error { Namespace: p.Namespace, FsSlice: p.FieldSpecs, SetRoleBindingSubjects: p.SetRoleBindingSubjects, - SkipExisting: p.SkipExisting, + UnsetOnly: p.UnsetOnly, }); err != nil { return err } diff --git a/plugin/builtin/namespacetransformer/NamespaceTransformer_test.go b/plugin/builtin/namespacetransformer/NamespaceTransformer_test.go index 4e051d16fab..1ba3bce99f4 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