Stop failing on single helmfile part missing specified env (#885)

* Stop failing on single helmfile part missing specified env

Ref https://github.com/helmfile/helmfile/issues/807

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

* Enhance RequireLog test helper to support updating snapshot

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

---------

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

pkg/app/desired_state_file_loader.go

@@ -160,7 +160,7 @@ func (a *desiredStateLoader) rawLoad(yaml []byte, baseDir, file string, evaluate
 	var st *state.HelmState
 	var err error
 	if runtime.V1Mode {
-		st, err = a.underlying().ParseAndLoad(yaml, baseDir, file, a.env, evaluateBases, env, overrodeEnv)
+		st, err = a.underlying().ParseAndLoad(yaml, baseDir, file, a.env, false, evaluateBases, env, overrodeEnv)
 		if err != nil {
 			return nil, err
 		}
@@ -170,7 +170,7 @@ func (a *desiredStateLoader) rawLoad(yaml []byte, baseDir, file string, evaluate
 			return nil, err
 		}
 
-		st, err = a.underlying().ParseAndLoad(yaml, baseDir, file, a.env, evaluateBases, merged, nil)
+		st, err = a.underlying().ParseAndLoad(yaml, baseDir, file, a.env, false, evaluateBases, merged, nil)
 		if err != nil {
 			return nil, err
 		}
@@ -243,5 +243,18 @@ func (ld *desiredStateLoader) load(env, overrodeEnv *environment.Environment, ba
 		ld.logger.Debugf("merged environment: %v", env)
 	}
 
+	// We defer the missing env detection and failure until
+	// all the helmfile parts are loaded and merged.
+	// Otherwise, any single helmfile part missing the env would fail the whole helmfile run.
+	// That's problematic, because each helmfile part is supposed to be incomplete, and
+	// they become complete only after merging all the parts.
+	// See https://github.com/helmfile/helmfile/issues/807 for the rationale of this.
+	if _, ok := finalState.Environments[env.Name]; evaluateBases && env.Name != state.DefaultEnv && !ok {
+		return nil, &state.StateLoadError{
+			Msg:   fmt.Sprintf("failed to read %s", finalState.FilePath),
+			Cause: &state.UndefinedEnvError{Env: env.Name},
+		}
+	}
+
 	return finalState, nil
 }

pkg/app/testdata/app_list_test/fail_on_unknown_environment

@@ -110,6 +110,7 @@ second-pass rendering result of "helmfile_1.yaml.part.0":
 47:   - test2
 48: 
 
+merged environment: &{staging  map[] map[]}
 changing working directory back to "/path/to"
 processing file "helmfile_2.yaml" in directory "/path/to/helmfile.d"
 changing working directory to "/path/to/helmfile.d"
@@ -168,6 +169,7 @@ second-pass rendering result of "helmfile_2.yaml.part.0":
 20:   version: 11.6.22
 21: 
 
+merged environment: &{staging  map[] map[]}
 changing working directory back to "/path/to"
 processing file "helmfile_3.yaml" in directory "/path/to/helmfile.d"
 changing working directory to "/path/to/helmfile.d"
@@ -194,4 +196,5 @@ second-pass rendering result of "helmfile_3.yaml.part.0":
  4:   namespace: kube-system
  5: 
 
+merged environment: &{staging  map[] map[]}
 changing working directory back to "/path/to"

pkg/app/testdata/app_list_test/filters_releases_for_environment_used_in_multiple_files

@@ -196,4 +196,5 @@ second-pass rendering result of "helmfile_3.yaml.part.0":
  4:   namespace: kube-system
  5: 
 
+merged environment: &{shared  map[] map[]}
 changing working directory back to "/path/to"

pkg/app/testdata/app_list_test/filters_releases_for_environment_used_in_one_file_only

@@ -110,6 +110,7 @@ second-pass rendering result of "helmfile_1.yaml.part.0":
 47:   - test2
 48: 
 
+merged environment: &{test  map[] map[]}
 changing working directory back to "/path/to"
 processing file "helmfile_2.yaml" in directory "/path/to/helmfile.d"
 changing working directory to "/path/to/helmfile.d"
@@ -195,4 +196,5 @@ second-pass rendering result of "helmfile_3.yaml.part.0":
  4:   namespace: kube-system
  5: 
 
+merged environment: &{test  map[] map[]}
 changing working directory back to "/path/to"

pkg/app/testdata/app_list_test/list_releases_matching_selector_and_environment

@@ -169,6 +169,7 @@ second-pass rendering result of "helmfile_2.yaml.part.0":
 20:   version: 11.6.22
 21: 
 
+merged environment: &{development  map[] map[]}
 changing working directory back to "/path/to"
 processing file "helmfile_3.yaml" in directory "/path/to/helmfile.d"
 changing working directory to "/path/to/helmfile.d"
@@ -195,4 +196,5 @@ second-pass rendering result of "helmfile_3.yaml.part.0":
  4:   namespace: kube-system
  5: 
 
+merged environment: &{development  map[] map[]}
 changing working directory back to "/path/to"

pkg/app/two_pass_renderer.go

@@ -53,7 +53,7 @@ func (r *desiredStateLoader) renderPrestate(firstPassEnv, overrode *environment.
 	c := r.underlying()
 	c.Strict = false
 	// create preliminary state, as we may have an environment. Tolerate errors.
-	prestate, err := c.ParseAndLoad([]byte(sanitized), baseDir, filename, r.env, false, firstPassEnv, overrode)
+	prestate, err := c.ParseAndLoad([]byte(sanitized), baseDir, filename, r.env, true, false, firstPassEnv, overrode)
 	if err != nil {
 		if _, ok := err.(*state.StateLoadError); ok {
 			r.logger.Debugf("could not deduce `environment:` block, configuring only .Environment.Name. error: %v", err)

pkg/state/create.go

@@ -22,20 +22,20 @@ const (
 )
 
 type StateLoadError struct {
-	msg   string
+	Msg   string
 	Cause error
 }
 
 func (e *StateLoadError) Error() string {
-	return fmt.Sprintf("%s: %v", e.msg, e.Cause)
+	return fmt.Sprintf("%s: %v", e.Msg, e.Cause)
 }
 
 type UndefinedEnvError struct {
-	msg string
+	Env string
 }
 
 func (e *UndefinedEnvError) Error() string {
-	return e.msg
+	return fmt.Sprintf("environment \"%s\" is not defined", e.Env)
 }
 
 type StateCreator struct {
@@ -138,7 +138,7 @@ func (c *StateCreator) Parse(content []byte, baseDir, file string) (*HelmState,
 }
 
 // LoadEnvValues loads environment values files relative to the `baseDir`
-func (c *StateCreator) LoadEnvValues(target *HelmState, env string, ctxEnv, overrode *environment.Environment, failOnMissingEnv bool) (*HelmState, error) {
+func (c *StateCreator) LoadEnvValues(target *HelmState, env string, failOnMissingEnv bool, ctxEnv, overrode *environment.Environment) (*HelmState, error) {
 	state := *target
 
 	e, err := c.loadEnvValues(&state, env, failOnMissingEnv, ctxEnv, overrode)
@@ -162,7 +162,7 @@ func (c *StateCreator) LoadEnvValues(target *HelmState, env string, ctxEnv, over
 
 // Parses YAML into HelmState, while loading environment values files relative to the `baseDir`
 // evaluateBases=true means that this is NOT a base helmfile
-func (c *StateCreator) ParseAndLoad(content []byte, baseDir, file string, envName string, evaluateBases bool, envValues, overrode *environment.Environment) (*HelmState, error) {
+func (c *StateCreator) ParseAndLoad(content []byte, baseDir, file string, envName string, failOnMissingEnv, evaluateBases bool, envValues, overrode *environment.Environment) (*HelmState, error) {
 	state, err := c.Parse(content, baseDir, file)
 	if err != nil {
 		return nil, err
@@ -179,7 +179,7 @@ func (c *StateCreator) ParseAndLoad(content []byte, baseDir, file string, envNam
 		}
 	}
 
-	state, err = c.LoadEnvValues(state, envName, envValues, overrode, evaluateBases)
+	state, err = c.LoadEnvValues(state, envName, failOnMissingEnv, envValues, overrode)
 	if err != nil {
 		return nil, err
 	}
@@ -249,7 +249,7 @@ func (c *StateCreator) loadEnvValues(st *HelmState, name string, failOnMissingEn
 			}
 		}
 	} else if ctxEnv == nil && name != DefaultEnv && failOnMissingEnv {
-		return nil, &UndefinedEnvError{msg: fmt.Sprintf("environment \"%s\" is not defined", name)}
+		return nil, &UndefinedEnvError{Env: name}
 	}
 
 	newEnv := &environment.Environment{Name: name, Values: envVals, KubeContext: envSpec.KubeContext}

pkg/state/create_test.go

@@ -20,7 +20,7 @@ func createFromYaml(content []byte, file string, env string, logger *zap.Sugared
 		fs:     filesystem.DefaultFileSystem(),
 		Strict: true,
 	}
-	return c.ParseAndLoad(content, filepath.Dir(file), file, env, true, nil, nil)
+	return c.ParseAndLoad(content, filepath.Dir(file), file, env, false, true, nil, nil)
 }
 
 func TestReadFromYaml(t *testing.T) {
@@ -54,9 +54,10 @@ func TestReadFromYaml_NonexistentEnv(t *testing.T) {
   chart: mychart
 `)
 	_, err := createFromYaml(yamlContent, yamlFile, "production", logger)
-	if err == nil {
-		t.Error("expected error")
-	}
+	// This does not produce an error because the environment existence check if done
+	// outside of the ParseAndLoad function since
+	// https://github.com/helmfile/helmfile/pull/885
+	require.NoError(t, err)
 }
 
 type stateTestEnv struct {
@@ -84,7 +85,7 @@ func (testEnv stateTestEnv) MustLoadStateWithEnableLiveOutput(t *testing.T, file
 
 	r := remote.NewRemote(logger, testFs.Cwd, testFs.ToFileSystem())
 	state, err := NewCreator(logger, testFs.ToFileSystem(), nil, nil, "", r, enableLiveOutput, "").
-		ParseAndLoad([]byte(yamlContent), filepath.Dir(file), file, envName, true, nil, nil)
+		ParseAndLoad([]byte(yamlContent), filepath.Dir(file), file, envName, true, true, nil, nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -154,7 +155,7 @@ releaseNamespace: mynamespace
 		Name: "production",
 	}
 	state, err := NewCreator(logger, testFs.ToFileSystem(), nil, nil, "", r, false, "").
-		ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, &env, nil)
+		ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, true, &env, nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -241,7 +242,7 @@ overrideNamespace: myns
 
 	r := remote.NewRemote(logger, testFs.Cwd, testFs.ToFileSystem())
 	state, err := NewCreator(logger, testFs.ToFileSystem(), nil, nil, "", r, false, "").
-		ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, nil, nil)
+		ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, true, nil, nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -525,7 +526,7 @@ releaseContext:
 
 	r := remote.NewRemote(logger, testFs.Cwd, testFs.ToFileSystem())
 	state, err := NewCreator(logger, testFs.ToFileSystem(), nil, nil, "", r, false, "").
-		ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, nil, nil)
+		ParseAndLoad(yamlContent, filepath.Dir(yamlFile), yamlFile, "production", true, true, nil, nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}

pkg/testhelper/require_log.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
 	"testing"
 )
@@ -11,6 +12,16 @@ import (
 func RequireLog(t *testing.T, dir string, bs *bytes.Buffer) {
 	t.Helper()
 
+	// Get the caller pkg used for instruction on rerunning the specific test
+	pc, _, _, _ := runtime.Caller(1)
+	funcName := runtime.FuncForPC(pc).Name()
+	lastSlash := strings.LastIndexByte(funcName, '/')
+	if lastSlash < 0 {
+		lastSlash = 0
+	}
+	firstDot := strings.IndexByte(funcName[lastSlash:], '.') + lastSlash
+	callerPkg := funcName[:firstDot]
+
 	testNameComponents := strings.Split(t.Name(), "/")
 	testBaseName := strings.ToLower(
 		strings.ReplaceAll(
@@ -21,21 +32,37 @@ func RequireLog(t *testing.T, dir string, bs *bytes.Buffer) {
 	)
 	wantLogFileDir := filepath.Join("testdata", dir)
 	wantLogFile := filepath.Join(wantLogFileDir, testBaseName)
-	wantLogData, err := os.ReadFile(wantLogFile)
-	updateLogFile := err != nil
-	wantLog := string(wantLogData)
-	gotLog := bs.String()
-	if updateLogFile {
+
+	if os.Getenv("HELMFILE_UPDATE_SNAPSHOT") != "" {
 		if err := os.MkdirAll(wantLogFileDir, 0755); err != nil {
 			t.Fatalf("unable to create directory %q: %v", wantLogFileDir, err)
 		}
 		if err := os.WriteFile(wantLogFile, bs.Bytes(), 0644); err != nil {
 			t.Fatalf("unable to update lint log snapshot: %v", err)
 		}
+		return
 	}
 
+	wantLogData, err := os.ReadFile(wantLogFile)
+	if err != nil {
+		t.Fatalf(
+			"Snapshot file %q does not exist. Rerun this test with `HELMFILE_UPDATE_SNAPSHOT=1 go test -v -run %s %s` to create the snapshot",
+			wantLogFile,
+			t.Name(),
+			callerPkg,
+		)
+	}
+
+	wantLog := string(wantLogData)
+	gotLog := bs.String()
+
 	diff, exists := Diff(wantLog, gotLog, 3)
 	if exists {
-		t.Errorf("unexpected log:\nDIFF\n%s\nEOD\nPlease remove %s and rerun the test to recapture this test snapshot", diff, wantLogFile)
+		t.Errorf("unexpected %s: want (-), got (+): %s", testBaseName, diff)
+		t.Errorf(
+			"If you think this is due to the snapshot file being outdated, rerun this test with `HELMFILE_UPDATE_SNAPSHOT=1 go test -v -run %s %s` to update the snapshot",
+			t.Name(),
+			callerPkg,
+		)
 	}
 }