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 <ykuoka@gmail.com>

* Fix helmfile template rendering error log for v1

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

* Add test for helmfile template debug log differences between v0 and v1 modes

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

* Fix helmfile template render debug log to not mention "first-pass" in v1 mode

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>

pkg/app/testdata/testreadfromyaml_rendertemplatelog/v0mode/log

@@ -0,0 +1,25 @@
+first-pass rendering starting for "": inherited=&{default map[] map[]}, overrode=<nil>
+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: 
+

pkg/app/testdata/testreadfromyaml_rendertemplatelog/v1mode/log

@@ -0,0 +1,10 @@
+rendering starting for "": inherited=&{default map[] map[]}, overrode=<nil>
+rendering result of "":
+ 0: 
+ 1: releases:
+ 2: - name: foo
+ 3:   chart: mychart1
+ 4: - name: bar
+ 5: 
+ 6: 
+

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 {
+	var (
-		r.logger.Debugf("first-pass uses: %v", initEnv)
+		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 {
+		finalEnv = initEnv
-		r.logger.Debugf("first-pass produced: %v", renderedEnv)
-	}
 
-	finalEnv, err := inherited.Merge(renderedEnv)
+		vals, err = finalEnv.GetMergedValues()
-	if err != nil {
+		if err != nil {
-		return nil, err
+			return nil, err
-	}
+		}
+	} else {
+		if r.logger != nil {
+			r.logger.Debugf("first-pass uses: %v", initEnv)
+		}
 
-	finalEnv, err = finalEnv.Merge(overrode)
+		renderedEnv, prestate := r.renderPrestate(initEnv, baseDir, filename, content)
-	if err != nil {
-		return nil, err
-	}
 
-	if r.logger != nil {
+		if r.logger != nil {
-		r.logger.Debugf("first-pass rendering result of \"%s\": %v", filename, *finalEnv)
+			r.logger.Debugf("first-pass produced: %v", renderedEnv)
-	}
+		}
 
-	vals, err := finalEnv.GetMergedValues()
+		mergedEnv, err := inherited.Merge(renderedEnv)
-	if err != nil {
+		if err != nil {
-		return nil, err
+			return nil, err
-	}
+		}
 
-	if prestate != nil {
+		mergedEnv, err = mergedEnv.Merge(overrode)
-		prestate.Env = *finalEnv
+		if err != nil {
-		r.logger.Debugf("vals:\n%v\ndefaultVals:%v", vals, prestate.DefaultValues)
+			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)
+	renderer := tmpl.NewFileRenderer(r.fs, baseDir, tmplData)
-	yamlBuf, err := secondPassRenderer.RenderTemplateContentToBuffer(content)
+	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
 }

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")