diff --git a/pkg/app/desired_state_file_loader.go b/pkg/app/desired_state_file_loader.go index f13a0ea7..c62a260c 100644 --- a/pkg/app/desired_state_file_loader.go +++ b/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 } diff --git a/pkg/app/testdata/app_list_test/fail_on_unknown_environment b/pkg/app/testdata/app_list_test/fail_on_unknown_environment index 74c00878..49d0fa69 100644 --- a/pkg/app/testdata/app_list_test/fail_on_unknown_environment +++ b/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" diff --git a/pkg/app/testdata/app_list_test/filters_releases_for_environment_used_in_multiple_files b/pkg/app/testdata/app_list_test/filters_releases_for_environment_used_in_multiple_files index 59d37d06..f8ddb46f 100644 --- a/pkg/app/testdata/app_list_test/filters_releases_for_environment_used_in_multiple_files +++ b/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" diff --git a/pkg/app/testdata/app_list_test/filters_releases_for_environment_used_in_one_file_only b/pkg/app/testdata/app_list_test/filters_releases_for_environment_used_in_one_file_only index 2dc4a454..a147faf3 100644 --- a/pkg/app/testdata/app_list_test/filters_releases_for_environment_used_in_one_file_only +++ b/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" diff --git a/pkg/app/testdata/app_list_test/list_releases_matching_selector_and_environment b/pkg/app/testdata/app_list_test/list_releases_matching_selector_and_environment index 81a89326..26c54e12 100644 --- a/pkg/app/testdata/app_list_test/list_releases_matching_selector_and_environment +++ b/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" diff --git a/pkg/app/two_pass_renderer.go b/pkg/app/two_pass_renderer.go index 79e8dcb4..735de02c 100644 --- a/pkg/app/two_pass_renderer.go +++ b/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) diff --git a/pkg/state/create.go b/pkg/state/create.go index efd4348f..0aea7638 100644 --- a/pkg/state/create.go +++ b/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} diff --git a/pkg/state/create_test.go b/pkg/state/create_test.go index e4b0c4b5..8b90cfa9 100644 --- a/pkg/state/create_test.go +++ b/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) } diff --git a/pkg/testhelper/require_log.go b/pkg/testhelper/require_log.go index 775fe537..40194105 100644 --- a/pkg/testhelper/require_log.go +++ b/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, + ) } }