From fc027d153885da1fa3e975c400dba43f29abd9b0 Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Tue, 7 Feb 2023 09:18:19 +0900 Subject: [PATCH] breaking: Fix the inherit feature to support multi-inheritance (#674) * breaking: Fix the inherit feature introduced in Helmfile v0.150.0 for multi-inheritance Follow-up for #435 Addresses https://github.com/helmfile/helmfile/discussions/656#discussioncomment-4877360 towards Helmfile v1 Signed-off-by: Yusuke Kuoka * Print a deprecation warning on releases[].inherit of map Signed-off-by: Yusuke Kuoka --------- Signed-off-by: Yusuke Kuoka --- pkg/app/app_template_test.go | 8 +- pkg/state/state.go | 19 +++- pkg/state/state_exec_tmpl.go | 106 +++++++++--------- pkg/state/temp_test.go | 12 +- .../release_template_inheritance/input.yaml | 8 +- 5 files changed, 86 insertions(+), 67 deletions(-) diff --git a/pkg/app/app_template_test.go b/pkg/app/app_template_test.go index 92483c79..9907a6af 100644 --- a/pkg/app/app_template_test.go +++ b/pkg/app/app_template_test.go @@ -435,23 +435,23 @@ func TestTemplate_CyclicInheritance(t *testing.T) { templates: a: inherit: - template: b + - template: b values: - a.yaml b: inherit: - template: c + - template: c values: - b.yaml c: inherit: - template: a + - template: a values: - c.yaml releases: - name: app1 inherit: - template: a + - template: a chart: incubator/raw `, } diff --git a/pkg/state/state.go b/pkg/state/state.go index edb6fc94..22145358 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -213,6 +213,8 @@ type Inherit struct { Except []string `yaml:"except,omitempty"` } +type Inherits []Inherit + // ReleaseSpec defines the structure of a helm release type ReleaseSpec struct { // Chart is the name of the chart being installed to create this release @@ -353,7 +355,22 @@ type ReleaseSpec struct { PostRenderer *string `yaml:"postRenderer,omitempty"` // Inherit is used to inherit a release template from a release or another release template - Inherit Inherit `yaml:"inherit,omitempty"` + Inherit Inherits `yaml:"inherit,omitempty"` +} + +func (r *Inherits) UnmarshalYAML(unmarshal func(interface{}) error) error { + var v0151 []Inherit + if err := unmarshal(&v0151); err != nil { + var v0150 Inherit + if err := unmarshal(&v0150); err != nil { + return err + } + fmt.Fprintf(os.Stderr, "releases[].inherit of map(%+v) has been deprecated and will be removed in v0.152.0. Wrap it into an array: %v\n", v0150, err) + *r = []Inherit{v0150} + return nil + } + *r = v0151 + return nil } // ChartPathOrName returns ChartPath if it is non-empty, and returns Chart otherwise. diff --git a/pkg/state/state_exec_tmpl.go b/pkg/state/state_exec_tmpl.go index 532b0199..c3976431 100644 --- a/pkg/state/state_exec_tmpl.go +++ b/pkg/state/state_exec_tmpl.go @@ -154,66 +154,68 @@ func (e CyclicReleaseTemplateInheritanceError) Error() string { // a cyclic relese template inheritance. // This functions fails with a CyclicReleaseTemplateInheritanceError if it finds a cyclic inheritance. func (st *HelmState) releaseWithInheritedTemplate(r *ReleaseSpec, inheritancePath []string) (*ReleaseSpec, error) { - templateName := r.Inherit.Template - if templateName == "" { - return r, nil - } - - path := append([]string{}, inheritancePath...) - path = append(path, templateName) - - var cycleFound bool - for _, t := range inheritancePath { - if t == templateName { - cycleFound = true - break - } - } - - if cycleFound { - return nil, CyclicReleaseTemplateInheritanceError{Message: fmt.Sprintf("cyclic inheritance detected: %s", strings.Join(path, "->"))} - } - - template, defined := st.Templates[templateName] - if !defined { - return nil, fmt.Errorf("release %q tried to inherit inexistent release template %q", r.Name, templateName) - } - - src, err := st.releaseWithInheritedTemplate(&template.ReleaseSpec, path) - if err != nil { - return nil, fmt.Errorf("unable to load release template %q: %w", templateName, err) - } - - for _, k := range r.Inherit.Except { - switch k { - case "labels": - src.Labels = map[string]string{} - case "values": - src.Values = nil - case "valuesTemplate": - src.ValuesTemplate = nil - case "setTemplate": - src.SetValuesTemplate = nil - case "set": - src.SetValues = nil - default: - return nil, fmt.Errorf("%q is not allowed under `inherit`. Allowed values are \"set\", \"setTemplate\", \"values\", \"valuesTemplate\", and \"labels\"", k) - } - - st.logger.Debugf("excluded field %q when inheriting template %q to release %q", k, templateName, r.Name) - } - var merged ReleaseSpec - if err := mergo.Merge(&merged, src, mergo.WithAppendSlice, mergo.WithSliceDeepCopy); err != nil { - return nil, fmt.Errorf("unable to inherit release template %q: %w", templateName, err) + for _, inherit := range r.Inherit { + templateName := inherit.Template + if templateName == "" { + return r, nil + } + + path := append([]string{}, inheritancePath...) + path = append(path, templateName) + + var cycleFound bool + for _, t := range inheritancePath { + if t == templateName { + cycleFound = true + break + } + } + + if cycleFound { + return nil, CyclicReleaseTemplateInheritanceError{Message: fmt.Sprintf("cyclic inheritance detected: %s", strings.Join(path, "->"))} + } + + template, defined := st.Templates[templateName] + if !defined { + return nil, fmt.Errorf("release %q tried to inherit inexistent release template %q", r.Name, templateName) + } + + src, err := st.releaseWithInheritedTemplate(&template.ReleaseSpec, path) + if err != nil { + return nil, fmt.Errorf("unable to load release template %q: %w", templateName, err) + } + + for _, k := range inherit.Except { + switch k { + case "labels": + src.Labels = map[string]string{} + case "values": + src.Values = nil + case "valuesTemplate": + src.ValuesTemplate = nil + case "setTemplate": + src.SetValuesTemplate = nil + case "set": + src.SetValues = nil + default: + return nil, fmt.Errorf("%q is not allowed under `inherit`. Allowed values are \"set\", \"setTemplate\", \"values\", \"valuesTemplate\", and \"labels\"", k) + } + + st.logger.Debugf("excluded field %q when inheriting template %q to release %q", k, templateName, r.Name) + } + + if err := mergo.Merge(&merged, src, mergo.WithAppendSlice, mergo.WithSliceDeepCopy); err != nil { + return nil, fmt.Errorf("unable to inherit release template %q: %w", templateName, err) + } } if err := mergo.Merge(&merged, r, mergo.WithAppendSlice, mergo.WithSliceDeepCopy); err != nil { return nil, fmt.Errorf("unable to load release %q: %w", r.Name, err) } - merged.Inherit = Inherit{} + merged.Inherit = nil return &merged, nil } diff --git a/pkg/state/temp_test.go b/pkg/state/temp_test.go index bcfc3c64..9637f8f5 100644 --- a/pkg/state/temp_test.go +++ b/pkg/state/temp_test.go @@ -38,39 +38,39 @@ func TestGenerateID(t *testing.T) { run(testcase{ subject: "baseline", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, - want: "foo-values-5d85cdbb5d", + want: "foo-values-7cd4757dfd", }) run(testcase{ subject: "different bytes content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: []byte(`{"k":"v"}`), - want: "foo-values-9548bdcd9", + want: "foo-values-7bb85f848f", }) run(testcase{ subject: "different map content", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"}, data: map[string]interface{}{"k": "v"}, - want: "foo-values-5db884fb87", + want: "foo-values-c57649655", }) run(testcase{ subject: "different chart", release: ReleaseSpec{Name: "foo", Chart: "stable/envoy"}, - want: "foo-values-6596c997cc", + want: "foo-values-bf798c8f", }) run(testcase{ subject: "different name", release: ReleaseSpec{Name: "bar", Chart: "incubator/raw"}, - want: "bar-values-5ccfd9f8b5", + want: "bar-values-5465f47b4f", }) run(testcase{ subject: "specific ns", release: ReleaseSpec{Name: "foo", Chart: "incubator/raw", Namespace: "myns"}, - want: "myns-foo-values-bc7cd5b87", + want: "myns-foo-values-55f77767f5", }) for id, n := range ids { diff --git a/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/input.yaml b/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/input.yaml index 3fa6a0ee..47e7a87c 100644 --- a/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/input.yaml +++ b/test/e2e/template/helmfile/testdata/snapshot/release_template_inheritance/input.yaml @@ -17,7 +17,7 @@ templates: labels: template1: template1 inherit: - template: base + - template: base except: - labels template2: @@ -29,14 +29,14 @@ templates: labels: template2: template2 inherit: - template: base + - template: base except: - valuesTemplate releases: - name: foo1 inherit: - template: template1 + - template: template1 values: - templates: - | @@ -49,7 +49,7 @@ releases: {{` {{ (unset .Values "templates") | toYaml | nindent 2 }} `}} - name: foo2 inherit: - template: template2 + - template: template2 values: - templates: - |