From bc6317122ba4af613a9580138b510196edc7f03c Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Tue, 24 Jan 2023 13:38:11 +0900 Subject: [PATCH] Disable double-rendering in V1 (#647) * Disable double-rendering in V1 We are going to force users to separate environments and releases sections in every helmfile.yaml in the [Helmfile V1](https://github.com/helmfile/helmfile/blob/main/docs/proposals/towards-1.0.md). The goal of separation was to make the helmfile.yaml rendering result not dependent on the double-rendering and therefore the feature should be safe to be turned off in V1. Signed-off-by: Yusuke Kuoka * Fix helmfile template rendering error log for v1 Signed-off-by: Yusuke Kuoka * Add test for helmfile template debug log differences between v0 and v1 modes Signed-off-by: Yusuke Kuoka * Fix helmfile template render debug log to not mention "first-pass" in v1 mode Signed-off-by: Yusuke Kuoka Signed-off-by: Yusuke Kuoka --- .../v0mode/log | 25 ++++++ .../v1mode/log | 10 +++ pkg/app/two_pass_renderer.go | 86 ++++++++++++------- pkg/app/two_pass_renderer_test.go | 82 ++++++++++++++++-- 4 files changed, 164 insertions(+), 39 deletions(-) create mode 100644 pkg/app/testdata/testreadfromyaml_rendertemplatelog/v0mode/log create mode 100644 pkg/app/testdata/testreadfromyaml_rendertemplatelog/v1mode/log diff --git a/pkg/app/testdata/testreadfromyaml_rendertemplatelog/v0mode/log b/pkg/app/testdata/testreadfromyaml_rendertemplatelog/v0mode/log new file mode 100644 index 00000000..99b5a047 --- /dev/null +++ b/pkg/app/testdata/testreadfromyaml_rendertemplatelog/v0mode/log @@ -0,0 +1,25 @@ +first-pass rendering starting for "": inherited=&{default map[] map[]}, overrode= +first-pass uses: &{default map[] map[]} +first-pass rendering output of "": + 0: + 1: releases: + 2: - name: foo + 3: chart: mychart1 + 4: - name: bar + 5: + 6: + +first-pass produced: &{default map[] map[]} +first-pass rendering result of "": {default map[] map[]} +vals: +map[] +defaultVals:[] +second-pass rendering result of "": + 0: + 1: releases: + 2: - name: foo + 3: chart: mychart1 + 4: - name: bar + 5: + 6: + diff --git a/pkg/app/testdata/testreadfromyaml_rendertemplatelog/v1mode/log b/pkg/app/testdata/testreadfromyaml_rendertemplatelog/v1mode/log new file mode 100644 index 00000000..f9d0c920 --- /dev/null +++ b/pkg/app/testdata/testreadfromyaml_rendertemplatelog/v1mode/log @@ -0,0 +1,10 @@ +rendering starting for "": inherited=&{default map[] map[]}, overrode= +rendering result of "": + 0: + 1: releases: + 2: - name: foo + 3: chart: mychart1 + 4: - name: bar + 5: + 6: + diff --git a/pkg/app/two_pass_renderer.go b/pkg/app/two_pass_renderer.go index 670fe4ea..af69b38b 100644 --- a/pkg/app/two_pass_renderer.go +++ b/pkg/app/two_pass_renderer.go @@ -8,6 +8,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/helmfile/helmfile/pkg/environment" + "github.com/helmfile/helmfile/pkg/runtime" "github.com/helmfile/helmfile/pkg/state" "github.com/helmfile/helmfile/pkg/tmpl" ) @@ -81,7 +82,11 @@ func (r *desiredStateLoader) renderTemplatesToYamlWithEnv(baseDir, filename stri func (r *desiredStateLoader) twoPassRenderTemplateToYaml(inherited, overrode *environment.Environment, baseDir, filename string, content []byte) (*bytes.Buffer, error) { // try a first pass render. This will always succeed, but can produce a limited env if r.logger != nil { - r.logger.Debugf("first-pass rendering starting for \"%s\": inherited=%v, overrode=%v", filename, inherited, overrode) + var phase string + if !runtime.V1Mode { + phase = "first-pass " + } + r.logger.Debugf("%srendering starting for \"%s\": inherited=%v, overrode=%v", phase, filename, inherited, overrode) } initEnv, err := inherited.Merge(overrode) @@ -89,51 +94,72 @@ func (r *desiredStateLoader) twoPassRenderTemplateToYaml(inherited, overrode *en return nil, err } - if r.logger != nil { - r.logger.Debugf("first-pass uses: %v", initEnv) - } + var ( + renderingPhase string + finalEnv *environment.Environment + vals map[string]interface{} + ) - renderedEnv, prestate := r.renderPrestate(initEnv, baseDir, filename, content) + if runtime.V1Mode { + var err error - if r.logger != nil { - r.logger.Debugf("first-pass produced: %v", renderedEnv) - } + finalEnv = initEnv - finalEnv, err := inherited.Merge(renderedEnv) - if err != nil { - return nil, err - } + vals, err = finalEnv.GetMergedValues() + if err != nil { + return nil, err + } + } else { + if r.logger != nil { + r.logger.Debugf("first-pass uses: %v", initEnv) + } - finalEnv, err = finalEnv.Merge(overrode) - if err != nil { - return nil, err - } + renderedEnv, prestate := r.renderPrestate(initEnv, baseDir, filename, content) - if r.logger != nil { - r.logger.Debugf("first-pass rendering result of \"%s\": %v", filename, *finalEnv) - } + if r.logger != nil { + r.logger.Debugf("first-pass produced: %v", renderedEnv) + } - vals, err := finalEnv.GetMergedValues() - if err != nil { - return nil, err - } + mergedEnv, err := inherited.Merge(renderedEnv) + if err != nil { + return nil, err + } - if prestate != nil { - prestate.Env = *finalEnv - r.logger.Debugf("vals:\n%v\ndefaultVals:%v", vals, prestate.DefaultValues) + mergedEnv, err = mergedEnv.Merge(overrode) + if err != nil { + return nil, err + } + + if r.logger != nil { + r.logger.Debugf("first-pass rendering result of \"%s\": %v", filename, *mergedEnv) + } + + renderingPhase = "second-pass " + + finalEnv = mergedEnv + + vals, err = finalEnv.GetMergedValues() + if err != nil { + return nil, err + } + + if prestate != nil { + prestate.Env = *mergedEnv + r.logger.Debugf("vals:\n%v\ndefaultVals:%v", vals, prestate.DefaultValues) + } } tmplData := state.NewEnvironmentTemplateData(*finalEnv, r.namespace, vals) - secondPassRenderer := tmpl.NewFileRenderer(r.fs, baseDir, tmplData) - yamlBuf, err := secondPassRenderer.RenderTemplateContentToBuffer(content) + renderer := tmpl.NewFileRenderer(r.fs, baseDir, tmplData) + yamlBuf, err := renderer.RenderTemplateContentToBuffer(content) if err != nil { if r.logger != nil { - r.logger.Debugf("second-pass rendering failed, input of \"%s\":\n%s", filename, prependLineNumbers(string(content))) + r.logger.Debugf("%srendering failed, input of \"%s\":\n%s", renderingPhase, filename, prependLineNumbers(string(content))) } return nil, err } if r.logger != nil { - r.logger.Debugf("second-pass rendering result of \"%s\":\n%s", filename, prependLineNumbers(yamlBuf.String())) + r.logger.Debugf("%srendering result of \"%s\":\n%s", renderingPhase, filename, prependLineNumbers(yamlBuf.String())) } return yamlBuf, nil } diff --git a/pkg/app/two_pass_renderer_test.go b/pkg/app/two_pass_renderer_test.go index 98d6f951..fde1630f 100644 --- a/pkg/app/two_pass_renderer_test.go +++ b/pkg/app/two_pass_renderer_test.go @@ -1,27 +1,32 @@ package app import ( + "bytes" "strings" "testing" + "github.com/helmfile/helmfile/pkg/helmexec" "github.com/helmfile/helmfile/pkg/remote" + "github.com/helmfile/helmfile/pkg/runtime" "github.com/helmfile/helmfile/pkg/state" "github.com/helmfile/helmfile/pkg/testhelper" "github.com/helmfile/helmfile/pkg/yaml" ) // nolint: unparam -func makeLoader(files map[string]string, env string) (*desiredStateLoader, *testhelper.TestFs) { +func makeLoader(files map[string]string, env string) (*desiredStateLoader, *testhelper.TestFs, *bytes.Buffer) { testfs := testhelper.NewTestFs(files) logger := newAppTestLogger() r := remote.NewRemote(logger, testfs.Cwd, testfs.ToFileSystem()) + var buf bytes.Buffer + loaderLogger := helmexec.NewLogger(&buf, "debug") return &desiredStateLoader{ env: env, namespace: "namespace", - logger: newAppTestLogger(), + logger: loaderLogger, fs: testfs.ToFileSystem(), remote: r, - }, testfs + }, testfs, &buf } func TestReadFromYaml_MakeEnvironmentHasNoSideEffects(t *testing.T) { @@ -42,7 +47,7 @@ releases: "/path/to/other/default/values.yaml": `SecondPass`, } - r, testfs := makeLoader(files, "staging") + r, testfs, _ := makeLoader(files, "staging") yamlBuf, err := r.renderTemplatesToYaml("", "", yamlContent) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -90,7 +95,7 @@ releases: "/path/to/default/values.yaml": defaultValuesYaml, } - r, _ := makeLoader(files, "staging") + r, _, _ := makeLoader(files, "staging") // test the double rendering yamlBuf, err := r.renderTemplatesToYaml("", "", yamlContent) if err != nil { @@ -117,6 +122,65 @@ releases: } } +func testReadFromYaml_RenderTemplateLog(t *testing.T) { + t.Helper() + + yamlContent := []byte(` +releases: +- name: foo + chart: mychart1 +- name: bar + +`) + + files := map[string]string{} + + r, _, logs := makeLoader(files, "default") + // test the double rendering + yamlBuf, err := r.renderTemplatesToYaml("", "", yamlContent) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + var state state.HelmState + err = yaml.Unmarshal(yamlBuf.Bytes(), &state) + + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + if len(state.Releases) != 2 { + t.Fatal("there should be 2 releases") + } + + if state.Releases[0].Name != "foo" { + t.Errorf("release name should be hello") + } + + if state.Releases[1].Name != "bar" { + t.Error("conditional release should have been present") + } + + assertLogEqualsToSnapshot(t, logs.String()) +} + +func TestReadFromYaml_RenderTemplateLog(t *testing.T) { + v := runtime.V1Mode + t.Cleanup(func() { + runtime.V1Mode = v + }) + + t.Run("v0mode", func(t *testing.T) { + runtime.V1Mode = false + testReadFromYaml_RenderTemplateLog(t) + }) + + t.Run("v1mode", func(t *testing.T) { + runtime.V1Mode = true + testReadFromYaml_RenderTemplateLog(t) + }) +} + func TestReadFromYaml_RenderTemplateWithValuesReferenceError(t *testing.T) { defaultValuesYaml := `` @@ -138,7 +202,7 @@ releases: "/path/to/default/values.yaml": defaultValuesYaml, } - r, _ := makeLoader(files, "staging") + r, _, _ := makeLoader(files, "staging") // test the double rendering _, err := r.renderTemplatesToYaml("", "", yamlContent) @@ -174,7 +238,7 @@ releases: "/path/to/values.yaml.gotmpl": defaultValuesYamlGotmpl, } - r, _ := makeLoader(files, "staging") + r, _, _ := makeLoader(files, "staging") rendered, _ := r.renderTemplatesToYaml("", "", yamlContent) var state state.HelmState @@ -200,7 +264,7 @@ func TestReadFromYaml_RenderTemplateWithNamespace(t *testing.T) { files := map[string]string{} - r, _ := makeLoader(files, "staging") + r, _, _ := makeLoader(files, "staging") yamlBuf, err := r.renderTemplatesToYaml("", "", yamlContent) if err != nil { t.Fatalf("unexpected error: %v", err) @@ -231,7 +295,7 @@ releases: chart: mychart `) - r, _ := makeLoader(map[string]string{}, "staging") + r, _, _ := makeLoader(map[string]string{}, "staging") _, err := r.renderTemplatesToYaml("", "", yamlContent) if err == nil { t.Fatalf("wanted error, none returned")