Fix helmfile deps not to remove entries for charts that are being chartified

When chartify is involved due to the use of `forceNamespace`, `strategicMergePatches`, `jsonPatches`, and so on, We had been internally mutating the Release.Chart with the path to the local temporary directory that contains the modified version of the chart.

This resulted in us unintentionally making `helmfile deps` to remove entries for the chart being modified out of helmfile.lock file, which resulted in issues like #2110.

To be clear, although the original issue is reported to occur for `strategicMergePatches`, I believe that it occurered also for any remote charts using `jsonPatches` and `forceNamespace` too.

I also believe this has been the issue since our introduction of chartify (maybe a year or so ago??), and I guess why it took so much time to be found and reported is that not so many people with chartify in combination with `helmfile deps` 🤔

Lastly, this changes chart names surfaced in the various log output from Helmfile, from temporary chart paths to the chart name/path declared in the helmfile.yaml. I think this is generally a good change, no fear of being a breaking change. But if anyone has any concern about that, please feel free to comment/report/etc.

Ref https://github.com/roboll/helmfile/issues/2110

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

pkg/app/run.go

@@ -80,8 +80,11 @@ func (r *Run) withPreparedCharts(helmfileCommand string, opts state.ChartPrepare
 			Namespace:   rel.Namespace,
 			KubeContext: rel.KubeContext,
 		}
-		if chart := releaseToChart[key]; chart != "" {
-			rel.Chart = chart
+		if chart := releaseToChart[key]; chart != rel.Chart {
+			// In this case we assume that the chart is downloaded and modified by Helmfile and chartify.
+			// So we take note of the local filesystem path to the modified version of the chart
+			// and use it later via the Release.ChartPathOrName() func.
+			rel.ChartPath = chart
 		}
 	}
 

pkg/state/state.go

@@ -183,6 +183,11 @@ type RepositorySpec struct {
 type ReleaseSpec struct {
 	// Chart is the name of the chart being installed to create this release
 	Chart string `yaml:"chart,omitempty"`
+
+	// ChartPath is the downloaded and modified version of the remote Chart specified by the Chart field.
+	// This field is empty when the release is going to use the remote chart as-is, without any modifications(e.g. chartify).
+	ChartPath string `yaml:"chartPath,omitempty"`
+
 	// Directory is an alias to Chart which may be of more fit when you want to use a local/remote directory containing
 	// K8s manifests or Kustomization as a chart
 	Directory string `yaml:"directory,omitempty"`
@@ -315,6 +320,16 @@ type ReleaseSpec struct {
 	SkipDeps *bool `yaml:"skipDeps,omitempty"`
 }
 
+// ChartPathOrName returns ChartPath if it is non-empty, and returns Chart otherwise.
+// This is useful to redirect helm commands like `helm template`, `helm diff`, and `helm upgrade --install` to
+// our modified version of the chart, in case the user configured Helmfile to do modify the chart before being passed to Helm.
+func (r ReleaseSpec) ChartPathOrName() string {
+	if r.ChartPath != "" {
+		return r.ChartPath
+	}
+	return r.Chart
+}
+
 type Release struct {
 	ReleaseSpec
 
@@ -804,7 +819,7 @@ func (st *HelmState) SyncReleases(affectedReleases *AffectedReleases, helm helme
 			for prep := range jobQueue {
 				release := prep.release
 				flags := prep.flags
-				chart := normalizeChart(st.basePath, release.Chart)
+				chart := normalizeChart(st.basePath, release.ChartPathOrName())
 				var relErr *ReleaseError
 				context := st.createHelmContext(release, workerIndex)
 
@@ -1396,7 +1411,7 @@ func (st *HelmState) TemplateReleases(helm helmexec.Interface, outputDir string,
 		}
 
 		if len(errs) == 0 {
-			if err := helm.TemplateRelease(release.Name, release.Chart, flags...); err != nil {
+			if err := helm.TemplateRelease(release.Name, release.ChartPathOrName(), flags...); err != nil {
 				errs = append(errs, err)
 			}
 		}
@@ -1572,7 +1587,7 @@ func (st *HelmState) LintReleases(helm helmexec.Interface, additionalValues []st
 		}
 
 		if len(errs) == 0 {
-			if err := helm.Lint(release.Name, release.Chart, flags...); err != nil {
+			if err := helm.Lint(release.Name, release.ChartPathOrName(), flags...); err != nil {
 				errs = append(errs, err)
 			}
 		}
@@ -1870,7 +1885,7 @@ func (st *HelmState) DiffReleases(helm helmexec.Interface, additionalValues []st
 				buf := &bytes.Buffer{}
 				if prep.upgradeDueToSkippedDiff {
 					results <- diffResult{release, &ReleaseError{ReleaseSpec: release, err: nil, Code: HelmDiffExitCodeChanged}, buf}
-				} else if err := helm.DiffRelease(st.createHelmContextWithWriter(release, buf), release.Name, normalizeChart(st.basePath, release.Chart), suppressDiff, flags...); err != nil {
+				} else if err := helm.DiffRelease(st.createHelmContextWithWriter(release, buf), release.Name, normalizeChart(st.basePath, release.ChartPathOrName()), suppressDiff, flags...); err != nil {
 					switch e := err.(type) {
 					case helmexec.ExitError:
 						// Propagate any non-zero exit status from the external command like `helm` that is failed under the hood

pkg/state/temp_test.go

@@ -38,39 +38,39 @@ func TestGenerateID(t *testing.T) {
 	run(testcase{
 		subject: "baseline",
 		release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"},
-		want:    "foo-values-6d97d4c7ff",
+		want:    "foo-values-789768d94b",
 	})
 
 	run(testcase{
 		subject: "different bytes content",
 		release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"},
 		data:    []byte(`{"k":"v"}`),
-		want:    "foo-values-74fcbfbd68",
+		want:    "foo-values-7d669b768f",
 	})
 
 	run(testcase{
 		subject: "different map content",
 		release: ReleaseSpec{Name: "foo", Chart: "incubator/raw"},
 		data:    map[string]interface{}{"k": "v"},
-		want:    "foo-values-7b954b6b6b",
+		want:    "foo-values-dd597bd55",
 	})
 
 	run(testcase{
 		subject: "different chart",
 		release: ReleaseSpec{Name: "foo", Chart: "stable/envoy"},
-		want:    "foo-values-5c54676568",
+		want:    "foo-values-5d6c9f7f59",
 	})
 
 	run(testcase{
 		subject: "different name",
 		release: ReleaseSpec{Name: "bar", Chart: "incubator/raw"},
-		want:    "bar-values-5fff459546",
+		want:    "bar-values-b6cdc56c",
 	})
 
 	run(testcase{
 		subject: "specific ns",
 		release: ReleaseSpec{Name: "foo", Chart: "incubator/raw", Namespace: "myns"},
-		want:    "myns-foo-values-cfd9959dd",
+		want:    "myns-foo-values-7cddc65b5",
 	})
 
 	for id, n := range ids {