fix: Various helmfile commands should not leave temp values files (#520)

Fixes #504

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
 }

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) {
+func (st *HelmState) generateTemporaryValuesFiles(values []interface{}, missingFileHandler *string) ([]string, error) {
-	flags := []string{}
+	generatedFiles := []string{}
-	if release.Namespace != "" {
+
-		flags = append(flags, "--namespace", release.Namespace)
+	for _, value := range values {
-	}
-	for _, value := range release.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) {
+			ok, err := st.fileExists(path)
-				if release.MissingFileHandler == nil || *release.MissingFileHandler == "Error" {
+			if err != nil {
-					return nil, err
+				return nil, err
-				} else if *release.MissingFileHandler == "Warn" {
+			}
+			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())
+			generatedFiles = append(generatedFiles, valfile.Name())
-			flags = append(flags, "--values", 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" {

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