From 3edc7d6f331ded66499eaac4a0dbe4f83d371412 Mon Sep 17 00:00:00 2001
From: KUOKA Yusuke <ykuoka@gmail.com>
Date: Mon, 1 Apr 2019 22:24:18 +0900
Subject: [PATCH] fix: `helmfile diff` should not leave temp values files
 (#525)

Fixes #503 once again. A follow-up to #520 which was incomplete.
---
 state/release.go    |  4 ++
 state/state.go      | 12 +++---
 state/state_test.go | 89 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+), 5 deletions(-)

diff --git a/state/release.go b/state/release.go
index 1c53ab71..77273a26 100644
--- a/state/release.go
+++ b/state/release.go
@@ -67,3 +67,7 @@ func (r ReleaseSpec) Clone() (*ReleaseSpec, error) {
 
 	return &deserialized, nil
 }
+
+func (r ReleaseSpec) Desired() bool {
+	return r.Installed == nil || *r.Installed
+}
diff --git a/state/state.go b/state/state.go
index cfe86041..fdfdb613 100644
--- a/state/state.go
+++ b/state/state.go
@@ -568,12 +568,14 @@ type diffPrepareResult struct {
 }
 
 func (st *HelmState) prepareDiffReleases(helm helmexec.Interface, additionalValues []string, concurrency int, detailedExitCode, suppressSecrets bool) ([]diffPrepareResult, []error) {
-	releases := []ReleaseSpec{}
-	for _, r := range st.Releases {
-		if r.Installed == nil || *r.Installed {
-			releases = append(releases, r)
+	releases := []*ReleaseSpec{}
+	for i, _ := range st.Releases {
+		if !st.Releases[i].Desired() {
+			continue
 		}
+		releases = append(releases, &st.Releases[i])
 	}
+
 	numReleases := len(releases)
 	jobs := make(chan *ReleaseSpec, numReleases)
 	results := make(chan diffPrepareResult, numReleases)
@@ -586,7 +588,7 @@ func (st *HelmState) prepareDiffReleases(helm helmexec.Interface, additionalValu
 		numReleases,
 		func() {
 			for i := 0; i < numReleases; i++ {
-				jobs <- &releases[i]
+				jobs <- releases[i]
 			}
 			close(jobs)
 		},
diff --git a/state/state_test.go b/state/state_test.go
index ee637a14..99fa8efb 100644
--- a/state/state_test.go
+++ b/state/state_test.go
@@ -973,6 +973,95 @@ func TestHelmState_SyncReleasesCleanup(t *testing.T) {
 	}
 }
 
+func TestHelmState_DiffReleasesCleanup(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.DiffReleases(tt.helm, []string{}, 1, false, false, false); 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",