From 8f2d97a1b3c949a9bbce4df69004b5cce4d6907d Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Thu, 9 Feb 2023 08:02:50 +0900 Subject: [PATCH] Relax the forbid-env-with-releases policy for plain helmfile.yaml on v1 (#684) --- docs/proposals/towards-1.0.md | 4 ++-- pkg/policy/checker.go | 11 ++++++----- pkg/policy/checker_test.go | 35 +++++++++++++++++++++++++++++++---- pkg/state/create.go | 2 ++ pkg/state/state.go | 2 +- 5 files changed, 42 insertions(+), 12 deletions(-) diff --git a/docs/proposals/towards-1.0.md b/docs/proposals/towards-1.0.md index a1745bd2..13b942c8 100644 --- a/docs/proposals/towards-1.0.md +++ b/docs/proposals/towards-1.0.md @@ -6,13 +6,13 @@ Note that every breaking change should have an easy alternative way to achieve t ## The changes in 1.0 -1. [Forbid the use of `environments` and `releases` within a single helmfile.yaml part](#forbid-the-use-of-environments-and-releases-within-a-single-helmfileyaml-part) +1. [Forbid the use of `environments` and `releases` within a single helmfile.yaml.gotmpl part](#forbid-the-use-of-environments-and-releases-within-a-single-helmfileyamlgotmpl-part) 2. [Force `.gotmpl` file extension for `helmfile.yaml` in case you want helmfile to render it as a go template before yaml parsing.](#force-gotmpl-file-extension-for-helmfileyaml-in-case-you-want-helmfile-to-render-it-as-a-go-template-before-yaml-parsing) 3. [Remove the `--args` flag from the `helmfile` command](#remove-the---args-flag-from-the-helmfile-command) 4. [Remove `HELMFILE_SKIP_INSECURE_TEMPLATE_FUNCTIONS` in favor of `HELMFILE_DISABLE_INSECURE_FEATURES`](#remove-helmfile_skip_insecure_template_functions-in-favor-of-helmfile_disable_insecure_features) 5. [The long deprecated `charts.yaml` has been finally removed](#the-long-deprecated-chartsyaml-has-been-finally-removed) -### Forbid the use of `environments` and `releases` within a single helmfile.yaml part +### Forbid the use of `environments` and `releases` within a single helmfile.yaml.gotmpl part - Helmfile currently relies on a hack called "double rendering" which no one understands correctly (I suppose) to solve the chicken-and-egg problem of rendering the helmfile template(which requires helmfile to parse `environments` as yaml first) and parsing the rendered helmfile as yaml(which requires helmfile to render the template first). - By forcing (or print a big warning) the user to do separate helmfile parts for `environments` and `releases`, it's very unlikely Helmfile needs double-rendering at all. diff --git a/pkg/policy/checker.go b/pkg/policy/checker.go index 81c1ba17..62d00f36 100644 --- a/pkg/policy/checker.go +++ b/pkg/policy/checker.go @@ -3,6 +3,7 @@ package policy import ( "errors" + "path/filepath" "github.com/helmfile/helmfile/pkg/runtime" ) @@ -12,13 +13,13 @@ var ( ) // checkerFunc is a function that checks the helmState. -type checkerFunc func(map[string]interface{}) (bool, error) +type checkerFunc func(string, map[string]interface{}) (bool, error) -func forbidEnvironmentsWithReleases(releaseState map[string]interface{}) (bool, error) { +func forbidEnvironmentsWithReleases(filePath string, releaseState map[string]interface{}) (bool, error) { // forbid environments and releases to be defined at the same yaml part _, hasEnvironments := releaseState["environments"] _, hasReleases := releaseState["releases"] - if hasEnvironments && hasReleases { + if hasEnvironments && hasReleases && (filepath.Ext(filePath) == ".gotmpl" || !runtime.V1Mode) { return runtime.V1Mode, EnvironmentsAndReleasesWithinSameYamlPartErr } return false, nil @@ -29,9 +30,9 @@ var checkerFuncs = []checkerFunc{ } // Checker is a policy checker for the helmfile state. -func Checker(helmState map[string]interface{}) (bool, error) { +func Checker(filePath string, helmState map[string]interface{}) (bool, error) { for _, fn := range checkerFuncs { - if isStrict, err := fn(helmState); err != nil { + if isStrict, err := fn(filePath, helmState); err != nil { return isStrict, err } } diff --git a/pkg/policy/checker_test.go b/pkg/policy/checker_test.go index 6ea35366..e6a8b28e 100644 --- a/pkg/policy/checker_test.go +++ b/pkg/policy/checker_test.go @@ -4,17 +4,23 @@ import ( "testing" "github.com/stretchr/testify/require" + + "github.com/helmfile/helmfile/pkg/runtime" ) func TestForbidEnvironmentsWithReleases(t *testing.T) { testCases := []struct { name string + filePath string + v1mode bool helmState map[string]interface{} expectedErr bool isStrict bool }{ { - name: "no error when only releases", + name: "no error when only releases", + filePath: "helmfile.yaml", + v1mode: false, helmState: map[string]interface{}{ "releases": interface{}(nil), }, @@ -22,7 +28,9 @@ func TestForbidEnvironmentsWithReleases(t *testing.T) { isStrict: false, }, { - name: "no error when only environments", + name: "no error when only environments", + filePath: "helmfile.yaml", + v1mode: false, helmState: map[string]interface{}{ "environments": map[string]interface{}{}, }, @@ -30,7 +38,9 @@ func TestForbidEnvironmentsWithReleases(t *testing.T) { isStrict: false, }, { - name: "error when both releases and environments", + name: "error when both releases and environments", + filePath: "helmfile.yaml", + v1mode: false, helmState: map[string]interface{}{ "environments": interface{}(nil), "releases": interface{}(nil), @@ -38,11 +48,28 @@ func TestForbidEnvironmentsWithReleases(t *testing.T) { expectedErr: true, isStrict: false, }, + { + name: "no error when both releases and environments for plain yaml on v1", + filePath: "helmfile.yaml", + v1mode: true, + helmState: map[string]interface{}{ + "environments": interface{}(nil), + "releases": interface{}(nil), + }, + expectedErr: false, + isStrict: false, + }, } + v1mode := runtime.V1Mode + t.Cleanup(func() { + runtime.V1Mode = v1mode + }) + for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - isStrict, err := forbidEnvironmentsWithReleases(tc.helmState) + runtime.V1Mode = tc.v1mode + isStrict, err := forbidEnvironmentsWithReleases(tc.filePath, tc.helmState) require.Equal(t, tc.isStrict, isStrict, "expected isStrict=%v, got=%v", tc.isStrict, isStrict) if tc.expectedErr { require.ErrorIsf(t, err, EnvironmentsAndReleasesWithinSameYamlPartErr, "expected error=%v, got=%v", EnvironmentsAndReleasesWithinSameYamlPartErr, err) diff --git a/pkg/state/create.go b/pkg/state/create.go index 4e475b34..bb1a6c5b 100644 --- a/pkg/state/create.go +++ b/pkg/state/create.go @@ -96,6 +96,8 @@ func (c *StateCreator) Parse(content []byte, baseDir, file string) (*HelmState, var intermediate HelmState + intermediate.FilePath = file + err := decode(&intermediate) if err == io.EOF { break diff --git a/pkg/state/state.go b/pkg/state/state.go index 22145358..38deb78a 100644 --- a/pkg/state/state.go +++ b/pkg/state/state.go @@ -104,7 +104,7 @@ func (hs *HelmState) UnmarshalYAML(unmarshal func(interface{}) error) error { return err } - isStrict, err := policy.Checker(helmStateInfo) + isStrict, err := policy.Checker(hs.FilePath, helmStateInfo) if err != nil { if isStrict { return err