From 283dac1531bb002155cc15dd1b19e35799e7835d Mon Sep 17 00:00:00 2001 From: KUOKA Yusuke Date: Sun, 31 Mar 2019 14:49:51 +0900 Subject: [PATCH] fix: Various helmfile commands should not leave temp values files (#520) Fixes #504 --- state/create.go | 12 ++++++ state/state.go | 80 ++++++++++++++++++++++++++++++---------- state/state_test.go | 90 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+), 19 deletions(-) diff --git a/state/create.go b/state/create.go index 808ad1b2..860fcf49 100644 --- a/state/create.go +++ b/state/create.go @@ -114,6 +114,18 @@ func (c *creator) CreateFromYaml(content []byte, file string, env string) (*Helm state.Env = *e state.readFile = c.readFile + state.removeFile = os.Remove + state.fileExists = func(path string) (bool, error) { + _, err := os.Stat(path) + + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, err + } + return true, nil + } return &state, nil } diff --git a/state/state.go b/state/state.go index b3d467c7..cfe86041 100644 --- a/state/state.go +++ b/state/state.go @@ -47,6 +47,9 @@ type HelmState struct { readFile func(string) ([]byte, error) + removeFile func(string) error + fileExists func(string) (bool, error) + runner helmexec.Runner } @@ -233,8 +236,11 @@ func (st *HelmState) prepareSyncReleases(helm helmexec.Interface, additionalValu errs = append(errs, &ReleaseError{release, err}) } - if _, err := os.Stat(valfile); os.IsNotExist(err) { + ok, err := st.fileExists(valfile) + if err != nil { errs = append(errs, &ReleaseError{release, err}) + } else if !ok { + errs = append(errs, &ReleaseError{release, fmt.Errorf("file does not exist: %s", valfile)}) } flags = append(flags, "--values", valfile) } @@ -749,7 +755,7 @@ func (st *HelmState) Clean() []error { for _, release := range st.Releases { for _, value := range release.generatedValues { - err := os.Remove(value) + err := st.removeFile(value) if err != nil { errs = append(errs, err) } @@ -1068,23 +1074,25 @@ func (st *HelmState) RenderValuesFileToBytes(path string) ([]byte, error) { return r.RenderToBytes(path) } -func (st *HelmState) namespaceAndValuesFlags(helm helmexec.Interface, release *ReleaseSpec) ([]string, error) { - flags := []string{} - if release.Namespace != "" { - flags = append(flags, "--namespace", release.Namespace) - } - for _, value := range release.Values { +func (st *HelmState) generateTemporaryValuesFiles(values []interface{}, missingFileHandler *string) ([]string, error) { + generatedFiles := []string{} + + for _, value := range values { switch typedValue := value.(type) { case string: - path := st.normalizePath(release.ValuesPathPrefix + typedValue) + path := st.normalizePath(typedValue) - if _, err := os.Stat(path); os.IsNotExist(err) { - if release.MissingFileHandler == nil || *release.MissingFileHandler == "Error" { - return nil, err - } else if *release.MissingFileHandler == "Warn" { + ok, err := st.fileExists(path) + if err != nil { + return nil, err + } + if !ok { + if missingFileHandler == nil || *missingFileHandler == "Error" { + return nil, fmt.Errorf("file does not exist: %s", path) + } else if *missingFileHandler == "Warn" { st.logger.Warnf("skipping missing values file \"%s\"", path) continue - } else if *release.MissingFileHandler == "Info" { + } else if *missingFileHandler == "Info" { st.logger.Infof("skipping missing values file \"%s\"", path) continue } else { @@ -1108,8 +1116,7 @@ func (st *HelmState) namespaceAndValuesFlags(helm helmexec.Interface, release *R return nil, fmt.Errorf("failed to write %s: %v", valfile.Name(), err) } st.logger.Debugf("successfully generated the value file at %s. produced:\n%s", path, string(yamlBytes)) - flags = append(flags, "--values", valfile.Name()) - + generatedFiles = append(generatedFiles, valfile.Name()) case map[interface{}]interface{}: valfile, err := ioutil.TempFile("", "values") if err != nil { @@ -1121,14 +1128,49 @@ func (st *HelmState) namespaceAndValuesFlags(helm helmexec.Interface, release *R if err := encoder.Encode(typedValue); err != nil { return nil, err } - release.generatedValues = append(release.generatedValues, valfile.Name()) - flags = append(flags, "--values", valfile.Name()) + generatedFiles = append(generatedFiles, valfile.Name()) + default: + return nil, fmt.Errorf("unexpected type of values entry: %T", typedValue) + } + } + return generatedFiles, nil +} + +func (st *HelmState) namespaceAndValuesFlags(helm helmexec.Interface, release *ReleaseSpec) ([]string, error) { + flags := []string{} + if release.Namespace != "" { + flags = append(flags, "--namespace", release.Namespace) + } + + values := []interface{}{} + for _, v := range release.Values { + switch typedValue := v.(type) { + case string: + path := st.normalizePath(release.ValuesPathPrefix + typedValue) + values = append(values, path) + default: + values = append(values, v) } } + generatedFiles, err := st.generateTemporaryValuesFiles(values, release.MissingFileHandler) + if err != nil { + return nil, err + } + + for _, f := range generatedFiles { + flags = append(flags, "--values", f) + } + + release.generatedValues = append(release.generatedValues, generatedFiles...) + for _, value := range release.Secrets { path := st.normalizePath(release.ValuesPathPrefix + value) - if _, err := os.Stat(path); os.IsNotExist(err) { + ok, err := st.fileExists(path) + if err != nil { + return nil, err + } + if !ok { if release.MissingFileHandler == nil || *release.MissingFileHandler == "Error" { return nil, err } else if *release.MissingFileHandler == "Warn" { diff --git a/state/state_test.go b/state/state_test.go index 05881eb8..ee637a14 100644 --- a/state/state_test.go +++ b/state/state_test.go @@ -8,6 +8,7 @@ import ( "errors" "strings" + "fmt" "github.com/roboll/helmfile/helmexec" ) @@ -883,6 +884,95 @@ func TestHelmState_SyncReleases(t *testing.T) { } } +func TestHelmState_SyncReleasesCleanup(t *testing.T) { + tests := []struct { + name string + releases []ReleaseSpec + helm *mockHelmExec + expectedNumRemovedFiles int + }{ + { + name: "normal release", + releases: []ReleaseSpec{ + { + Name: "releaseName", + Chart: "foo", + }, + }, + helm: &mockHelmExec{}, + expectedNumRemovedFiles: 0, + }, + { + name: "inline values", + releases: []ReleaseSpec{ + { + Name: "releaseName", + Chart: "foo", + Values: []interface{}{ + map[interface{}]interface{}{ + "someList": "a,b,c", + }, + }, + }, + }, + helm: &mockHelmExec{}, + expectedNumRemovedFiles: 1, + }, + { + name: "inline values and values file", + releases: []ReleaseSpec{ + { + Name: "releaseName", + Chart: "foo", + Values: []interface{}{ + map[interface{}]interface{}{ + "someList": "a,b,c", + }, + "someFile", + }, + }, + }, + helm: &mockHelmExec{}, + expectedNumRemovedFiles: 2, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + numRemovedFiles := 0 + state := &HelmState{ + Releases: tt.releases, + logger: logger, + readFile: func(f string) ([]byte, error) { + if f != "someFile" { + return nil, fmt.Errorf("unexpected file to read: %s", f) + } + someFileContent := []byte(`foo: bar +`) + return someFileContent, nil + }, + removeFile: func(f string) error { + numRemovedFiles += 1 + return nil + }, + fileExists: func(f string) (bool, error) { + return true, nil + }, + } + if errs := state.SyncReleases(tt.helm, []string{}, 1); errs != nil && len(errs) > 0 { + t.Errorf("unexpected errors: %v", errs) + } + + if errs := state.Clean(); errs != nil && len(errs) > 0 { + t.Errorf("unexpected errors: %v", errs) + } + + if numRemovedFiles != tt.expectedNumRemovedFiles { + t.Errorf("unexpected number of removed files: expected %d, got %d", tt.expectedNumRemovedFiles, numRemovedFiles) + } + }) + } +} + func TestHelmState_UpdateDeps(t *testing.T) { state := &HelmState{ basePath: "/src",