From 9a27a9f19f4ce9029a371460b026555b8ad77ed1 Mon Sep 17 00:00:00 2001 From: Natasha Sarkar Date: Thu, 29 Jul 2021 15:46:09 -0700 Subject: [PATCH] replace genargs with two separate annotations --- api/internal/plugins/utils/utils.go | 8 +- api/internal/utils/makeResIds.go | 5 +- api/resource/factory.go | 19 ++-- api/resource/resource.go | 88 +++++++++---------- api/resource/resource_test.go | 58 ------------ api/types/genargs.go | 61 ------------- api/types/genargs_test.go | 39 -------- .../bashedconfigmap/BashedConfigMap_test.go | 4 +- 8 files changed, 62 insertions(+), 220 deletions(-) delete mode 100644 api/types/genargs.go delete mode 100644 api/types/genargs_test.go diff --git a/api/internal/plugins/utils/utils.go b/api/internal/plugins/utils/utils.go index d1c2af766..8182f203e 100644 --- a/api/internal/plugins/utils/utils.go +++ b/api/internal/plugins/utils/utils.go @@ -231,10 +231,10 @@ func UpdateResourceOptions(rm resmap.ResMap) (resmap.ResMap, error) { if err := r.SetAnnotations(annotations); err != nil { return nil, err } - r.SetOptions(types.NewGenArgs( - &types.GeneratorArgs{ - Behavior: behavior, - Options: &types.GeneratorOptions{DisableNameSuffixHash: !needsHash}})) + if needsHash { + r.EnableHashSuffix() + } + r.SetBehavior(types.NewGenerationBehavior(behavior)) } return rm, nil } diff --git a/api/internal/utils/makeResIds.go b/api/internal/utils/makeResIds.go index 455a14987..0a1ea0158 100644 --- a/api/internal/utils/makeResIds.go +++ b/api/internal/utils/makeResIds.go @@ -16,13 +16,14 @@ const ( BuildAnnotationPrefixes = konfig.ConfigAnnoDomain + "/prefixes" BuildAnnotationSuffixes = konfig.ConfigAnnoDomain + "/suffixes" BuildAnnotationsRefBy = konfig.ConfigAnnoDomain + "/refBy" - BuildAnnotationsGenOptions = konfig.ConfigAnnoDomain + "/generatorOptions" + BuildAnnotationsGenBehavior = konfig.ConfigAnnoDomain + "/generatorBehavior" + BuildAnnotationsGenAddHashSuffix = konfig.ConfigAnnoDomain + "/needsHashSuffix" // the following are only for patches, to specify whether they can change names // and kinds of their targets BuildAnnotationAllowNameChange = konfig.ConfigAnnoDomain + "/allowNameChange" BuildAnnotationAllowKindChange = konfig.ConfigAnnoDomain + "/allowKindChange" - Allowed = "allowed" + Enabled = "enabled" ) // MakeResIds returns all of an RNode's current and previous Ids diff --git a/api/resource/factory.go b/api/resource/factory.go index d01e21b6b..fb01bc8ce 100644 --- a/api/resource/factory.go +++ b/api/resource/factory.go @@ -64,19 +64,22 @@ func (rf *Factory) FromMapAndOption( // TODO: return err instead of log. log.Fatal(err) } - return rf.makeOne(n, types.NewGenArgs(args)) + return rf.makeOne(n, args) } // makeOne returns a new instance of Resource. -func (rf *Factory) makeOne(rn *yaml.RNode, o *types.GenArgs) *Resource { +func (rf *Factory) makeOne(rn *yaml.RNode, o *types.GeneratorArgs) *Resource { if rn == nil { log.Fatal("RNode must not be null") } - if o == nil { - o = types.NewGenArgs(nil) - } resource := &Resource{RNode: *rn} - resource.SetOptions(o) + if o != nil { + if o.Options == nil || !o.Options.DisableNameSuffixHash { + resource.EnableHashSuffix() + } + resource.SetBehavior(types.NewGenerationBehavior(o.Behavior)) + } + return resource } @@ -267,7 +270,7 @@ func (rf *Factory) MakeConfigMap(kvLdr ifc.KvLoader, args *types.ConfigMapArgs) if err != nil { return nil, err } - return rf.makeOne(rn, types.NewGenArgs(&args.GeneratorArgs)), nil + return rf.makeOne(rn, &args.GeneratorArgs), nil } // MakeSecret makes an instance of Resource for Secret @@ -276,5 +279,5 @@ func (rf *Factory) MakeSecret(kvLdr ifc.KvLoader, args *types.SecretArgs) (*Reso if err != nil { return nil, err } - return rf.makeOne(rn, types.NewGenArgs(&args.GeneratorArgs)), nil + return rf.makeOne(rn, &args.GeneratorArgs), nil } diff --git a/api/resource/resource.go b/api/resource/resource.go index e8c19161a..3778f613a 100644 --- a/api/resource/resource.go +++ b/api/resource/resource.go @@ -35,7 +35,8 @@ var BuildAnnotations = []string{ utils.BuildAnnotationAllowNameChange, utils.BuildAnnotationAllowKindChange, utils.BuildAnnotationsRefBy, - utils.BuildAnnotationsGenOptions, + utils.BuildAnnotationsGenBehavior, + utils.BuildAnnotationsGenAddHashSuffix, kioutil.PathAnnotation, kioutil.IndexAnnotation, @@ -246,34 +247,38 @@ func (r *Resource) setPreviousId(ns string, n string, k string) *Resource { // AllowNameChange allows name changes to the resource. func (r *Resource) AllowNameChange() { - annotations := r.GetAnnotations() - annotations[utils.BuildAnnotationAllowNameChange] = utils.Allowed - if err := r.SetAnnotations(annotations); err != nil { - panic(err) - } + r.enable(utils.BuildAnnotationAllowNameChange) } +// NameChangeAllowed checks if a patch resource is allowed to change another resource's name. func (r *Resource) NameChangeAllowed() bool { - annotations := r.GetAnnotations() - v, ok := annotations[utils.BuildAnnotationAllowNameChange] - return ok && v == utils.Allowed + return r.isEnabled(utils.BuildAnnotationAllowNameChange) } // AllowKindChange allows kind changes to the resource. func (r *Resource) AllowKindChange() { + r.enable(utils.BuildAnnotationAllowKindChange) +} + +// KindChangeAllowed checks if a patch resource is allowed to change another resource's kind. +func (r *Resource) KindChangeAllowed() bool { + return r.isEnabled(utils.BuildAnnotationAllowKindChange) +} + +func (r *Resource) isEnabled(annoKey string) bool { annotations := r.GetAnnotations() - annotations[utils.BuildAnnotationAllowKindChange] = utils.Allowed + v, ok := annotations[annoKey] + return ok && v == utils.Enabled +} + +func (r *Resource) enable(annoKey string) { + annotations := r.GetAnnotations() + annotations[annoKey] = utils.Enabled if err := r.SetAnnotations(annotations); err != nil { panic(err) } } -func (r *Resource) KindChangeAllowed() bool { - annotations := r.GetAnnotations() - v, ok := annotations[utils.BuildAnnotationAllowKindChange] - return ok && v == utils.Allowed -} - // String returns resource as JSON. func (r *Resource) String() string { bs, err := r.MarshalJSON() @@ -302,43 +307,34 @@ func (r *Resource) MustYaml() string { return string(yml) } -func (r *Resource) getGenArgs() *types.GenArgs { - annotations := r.GetAnnotations() - if genOptsAnno, ok := annotations[utils.BuildAnnotationsGenOptions]; ok { - var genOpts types.GeneratorArgs - yaml.Unmarshal([]byte(genOptsAnno), &genOpts) - return types.NewGenArgs(&genOpts) - } - return nil -} - -// SetOptions updates the generator options for the resource. -func (r *Resource) SetOptions(o *types.GenArgs) { - annotations := r.GetAnnotations() - if o.IsNilOrEmpty() { - if len(annotations) == 0 { - return - } - if o == nil { - delete(annotations, utils.BuildAnnotationsGenOptions) - } - } else { - b, _ := o.AsYaml() - annotations[utils.BuildAnnotationsGenOptions] = string(b) - } - r.SetAnnotations(annotations) -} - // Behavior returns the behavior for the resource. func (r *Resource) Behavior() types.GenerationBehavior { - return r.getGenArgs().Behavior() + annotations := r.GetAnnotations() + if v, ok := annotations[utils.BuildAnnotationsGenBehavior]; ok { + return types.NewGenerationBehavior(v) + } + return types.NewGenerationBehavior("") +} + +// SetBehavior sets the behavior for the resource. +func (r *Resource) SetBehavior(behavior types.GenerationBehavior) { + annotations := r.GetAnnotations() + annotations[utils.BuildAnnotationsGenBehavior] = behavior.String() + if err := r.SetAnnotations(annotations); err != nil { + panic(err) + } } // NeedHashSuffix returns true if a resource content // hash should be appended to the name of the resource. func (r *Resource) NeedHashSuffix() bool { - options := r.getGenArgs() - return options != nil && options.ShouldAddHashSuffixToName() + return r.isEnabled(utils.BuildAnnotationsGenAddHashSuffix) +} + +// EnableHashSuffix marks the resource as needing a content +// hash to be appended to the name of the resource. +func (r *Resource) EnableHashSuffix() { + r.enable(utils.BuildAnnotationsGenAddHashSuffix) } // OrgId returns the original, immutable ResId for the resource. diff --git a/api/resource/resource_test.go b/api/resource/resource_test.go index a73437b9f..cf36fe1e4 100644 --- a/api/resource/resource_test.go +++ b/api/resource/resource_test.go @@ -30,8 +30,6 @@ var testConfigMap = factory.FromMap( //nolint:gosec const configMapAsString = `{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"winnie","namespace":"hundred-acre-wood"}}` -const configMapAsStringWithOptions = `{"apiVersion":"v1","kind":"ConfigMap","metadata":{"annotations":` + - `{"internal.config.kubernetes.io/generatorOptions":"{}\n"},"name":"winnie","namespace":"hundred-acre-wood"}}` var testDeployment = factory.FromMap( map[string]interface{}{ @@ -43,8 +41,6 @@ var testDeployment = factory.FromMap( }) const deploymentAsString = `{"apiVersion":"apps/v1","kind":"Deployment","metadata":{"name":"pooh"}}` -const deploymentAsStringWithOptions = `{"apiVersion":"apps/v1","kind":"Deployment","metadata":{"annotations":` + - `{"internal.config.kubernetes.io/generatorOptions":"{}\n"},"name":"pooh"}}` func TestAsYAML(t *testing.T) { expected := `apiVersion: apps/v1 @@ -80,28 +76,6 @@ func TestResourceString(t *testing.T) { } } -func TestResourceStringWithOptionsAnnotations(t *testing.T) { - tests := []struct { - in *Resource - s string - }{ - { - in: testConfigMap, - s: configMapAsStringWithOptions, - }, - { - in: testDeployment, - s: deploymentAsStringWithOptions, - }, - } - for _, test := range tests { - args := &types.GeneratorArgs{} - options := types.NewGenArgs(args) - test.in.SetOptions(options) - assert.Equal(t, test.in.String(), test.s) - } -} - func TestResourceId(t *testing.T) { tests := []struct { in *Resource @@ -1193,35 +1167,3 @@ spec: resid.FromString("gr2_ver2_knd2|ns2|name2"), }) } - -func TestOptions(t *testing.T) { - r, err := factory.FromBytes([]byte(` -apiVersion: v1 -kind: ConfigMap -metadata: - name: example-configmap-test -`)) - assert.NoError(t, err) - - args := &types.GeneratorArgs{ - Behavior: "merge", - Options: &types.GeneratorOptions{ - DisableNameSuffixHash: true, - }, - } - - options := types.NewGenArgs(args) - r.SetOptions(options) - assert.Equal(t, r.RNode.MustString(), `apiVersion: v1 -kind: ConfigMap -metadata: - name: example-configmap-test - annotations: - internal.config.kubernetes.io/generatorOptions: | - behavior: merge - options: - disableNameSuffixHash: true -`) - assert.Equal(t, r.Behavior(), types.BehaviorMerge) - assert.Equal(t, r.NeedHashSuffix(), !args.Options.DisableNameSuffixHash) -} diff --git a/api/types/genargs.go b/api/types/genargs.go deleted file mode 100644 index d68d924cc..000000000 --- a/api/types/genargs.go +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright 2019 The Kubernetes Authors. -// SPDX-License-Identifier: Apache-2.0 - -package types - -import ( - "strconv" - "strings" - - "sigs.k8s.io/kustomize/kyaml/yaml" -) - -// GenArgs is a facade over GeneratorArgs, exposing a few readonly properties. -type GenArgs struct { - args *GeneratorArgs -} - -// NewGenArgs returns a new instance of GenArgs. -func NewGenArgs(args *GeneratorArgs) *GenArgs { - return &GenArgs{args: args} -} - -func (g *GenArgs) String() string { - if g == nil { - return "{nilGenArgs}" - } - return "{" + - strings.Join([]string{ - "nsfx:" + strconv.FormatBool(g.ShouldAddHashSuffixToName()), - "beh:" + g.Behavior().String()}, - ",") + - "}" -} - -// ShouldAddHashSuffixToName returns true if a resource -// content hash should be appended to the name of the resource. -func (g *GenArgs) ShouldAddHashSuffixToName() bool { - return g.args != nil && - (g.args.Options == nil || !g.args.Options.DisableNameSuffixHash) -} - -// Behavior returns Behavior field of GeneratorArgs -func (g *GenArgs) Behavior() GenerationBehavior { - if g == nil || g.args == nil { - return BehaviorUnspecified - } - return NewGenerationBehavior(g.args.Behavior) -} - -// IsNilOrEmpty returns true if g is nil or if the args are empty -func (g *GenArgs) IsNilOrEmpty() bool { - return g == nil || g.args == nil -} - -// AsYaml returns a yaml marshalling of the underlying Genargs -func (g *GenArgs) AsYaml() ([]byte, error) { - if g == nil { - return yaml.Marshal(nil) - } - return yaml.Marshal(g.args) -} diff --git a/api/types/genargs_test.go b/api/types/genargs_test.go deleted file mode 100644 index 4d2a0cbee..000000000 --- a/api/types/genargs_test.go +++ /dev/null @@ -1,39 +0,0 @@ -// Copyright 2019 The Kubernetes Authors. -// SPDX-License-Identifier: Apache-2.0 - -package types_test - -import ( - "testing" - - . "sigs.k8s.io/kustomize/api/types" -) - -func TestGenArgs_String(t *testing.T) { - tests := []struct { - ga *GenArgs - expected string - }{ - { - ga: nil, - expected: "{nilGenArgs}", - }, - { - ga: &GenArgs{}, - expected: "{nsfx:false,beh:unspecified}", - }, - { - ga: NewGenArgs( - &GeneratorArgs{ - Behavior: "merge", - Options: &GeneratorOptions{DisableNameSuffixHash: false}, - }), - expected: "{nsfx:true,beh:merge}", - }, - } - for _, test := range tests { - if test.ga.String() != test.expected { - t.Fatalf("Expected '%s', got '%s'", test.expected, test.ga.String()) - } - } -} diff --git a/plugin/someteam.example.com/v1/bashedconfigmap/BashedConfigMap_test.go b/plugin/someteam.example.com/v1/bashedconfigmap/BashedConfigMap_test.go index 73a93d683..13c37db34 100644 --- a/plugin/someteam.example.com/v1/bashedconfigmap/BashedConfigMap_test.go +++ b/plugin/someteam.example.com/v1/bashedconfigmap/BashedConfigMap_test.go @@ -29,8 +29,8 @@ data: kind: ConfigMap metadata: annotations: - internal.config.kubernetes.io/generatorOptions: | - options: {} + internal.config.kubernetes.io/generatorBehavior: unspecified + internal.config.kubernetes.io/needsHashSuffix: enabled name: example-configmap-test `) if m.Resources()[0].NeedHashSuffix() != true {