Fix go-getter URL in chart to actually work (#1405)

Fixes #1401

pkg/remote/remote_test.go

@@ -2,6 +2,7 @@ package remote
 
 import (
 	"fmt"
+	"github.com/google/go-cmp/cmp"
 	"github.com/roboll/helmfile/pkg/helmexec"
 	"github.com/roboll/helmfile/pkg/testhelper"
 	"os"
@@ -156,6 +157,75 @@ func TestRemote_SShGitHub(t *testing.T) {
 	}
 }
 
+func TestParse(t *testing.T) {
+	type testcase struct {
+		input                            string
+		getter, scheme, dir, file, query string
+		err                              string
+	}
+
+	testcases := []testcase{
+		{
+			input: "raw/incubator",
+			err:   "parse url: missing scheme - probably this is a local file path? raw/incubator",
+		},
+		{
+			input:  "git::https://github.com/stakater/Forecastle.git@deployments/kubernetes/chart/forecastle?ref=v1.0.54",
+			getter: "git",
+			scheme: "https",
+			dir:    "/stakater/Forecastle.git",
+			file:   "deployments/kubernetes/chart/forecastle",
+			query:  "ref=v1.0.54",
+		},
+	}
+
+	for i := range testcases {
+		tc := testcases[i]
+
+		t.Run(fmt.Sprintf("case %d", i), func(t *testing.T) {
+			src, err := Parse(tc.input)
+
+			var errMsg string
+			if err != nil {
+				errMsg = err.Error()
+			}
+
+			if diff := cmp.Diff(tc.err, errMsg); diff != "" {
+				t.Fatalf("Unexpected error:\n%s", diff)
+			}
+
+			var getter, scheme, dir, file, query string
+			if src != nil {
+				getter = src.Getter
+				scheme = src.Scheme
+				dir = src.Dir
+				file = src.File
+				query = src.RawQuery
+			}
+
+			if diff := cmp.Diff(tc.getter, getter); diff != "" {
+				t.Fatalf("Unexpected getter:\n%s", diff)
+			}
+
+			if diff := cmp.Diff(tc.scheme, scheme); diff != "" {
+				t.Fatalf("Unexpected scheme:\n%s", diff)
+			}
+
+			if diff := cmp.Diff(tc.file, file); diff != "" {
+				t.Fatalf("Unexpected file:\n%s", diff)
+			}
+
+			if diff := cmp.Diff(tc.dir, dir); diff != "" {
+				t.Fatalf("Unexpected dir:\n%s", diff)
+			}
+
+			if diff := cmp.Diff(tc.query, query); diff != "" {
+				t.Fatalf("Unexpected query:\n%s", diff)
+			}
+		})
+	}
+}
+
 type testGetter struct {
 	get func(wd, src, dst string) error
 }

pkg/state/helmx.go

@@ -36,11 +36,34 @@ func directoryExistsAt(path string) bool {
 
 type Chartify struct {
 	Opts  *chartify.ChartifyOpts
-	Chart string
 	Clean func()
 }
 
-func (st *HelmState) PrepareChartify(helm helmexec.Interface, release *ReleaseSpec, workerIndex int) (*Chartify, func(), error) {
+func (st *HelmState) goGetterChart(chart, dir string, force bool) (string, error) {
+	if dir != "" && chart == "" {
+		chart = dir
+	}
+
+	_, err := remote.Parse(chart)
+	if err != nil {
+		if force {
+			return "", fmt.Errorf("Parsing url from dir failed due to error %q.\nContinuing the process assuming this is a regular Helm chart or a local dir.", err.Error())
+		}
+	} else {
+		r := remote.NewRemote(st.logger, st.basePath, st.readFile, directoryExistsAt, fileExistsAt)
+
+		fetchedDir, err := r.Fetch(chart)
+		if err != nil {
+			return "", fmt.Errorf("fetching %q: %v", chart, err)
+		}
+
+		chart = fetchedDir
+	}
+
+	return chart, nil
+}
+
+func (st *HelmState) PrepareChartify(helm helmexec.Interface, release *ReleaseSpec, chart string, workerIndex int) (*Chartify, func(), error) {
 	chartify := &Chartify{
 		Opts: &chartify.ChartifyOpts{
 			WorkaroundOutputDirIssue:    true,
@@ -58,31 +81,6 @@ func (st *HelmState) PrepareChartify(helm helmexec.Interface, release *ReleaseSp
 
 	var shouldRun bool
 
-	chart := release.Chart
-	if release.Directory != "" && chart == "" {
-		chart = release.Directory
-	}
-
-	_, err := remote.Parse(chart)
-	if err != nil {
-		if release.ForceGoGetter {
-			return nil, clean, fmt.Errorf("Parsing url from directory of release %q failed due to error %q.\nContinuing the process assuming this is a regular Helm chart or a local directory.", release.Name, err.Error())
-		}
-	} else {
-		r := remote.NewRemote(st.logger, st.basePath, st.readFile, directoryExistsAt, fileExistsAt)
-
-		fetchedDir, err := r.Fetch(chart)
-		if err != nil {
-			return nil, clean, fmt.Errorf("fetching %q: %v", chart, err)
-		}
-
-		chart = fetchedDir
-
-		filesNeedCleaning = append(filesNeedCleaning, fetchedDir)
-	}
-
-	chartify.Chart = chart
-
 	dir := filepath.Join(st.basePath, chart)
 	if stat, _ := os.Stat(dir); stat != nil && stat.IsDir() {
 		if exists, err := st.fileExists(filepath.Join(dir, "Chart.yaml")); err == nil && !exists {

pkg/state/state.go

@@ -859,9 +859,15 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre
 					return
 				}
 
-				var chartPath string
+				chartName := release.Chart
 
-				chartification, clean, err := st.PrepareChartify(helm, release, workerIndex)
+				chartPath, err := st.goGetterChart(chartName, release.Directory, release.ForceGoGetter)
+				if err != nil {
+					results <- &downloadResults{err: fmt.Errorf("release %q: %w", release.Name, err)}
+					return
+				}
+
+				chartification, clean, err := st.PrepareChartify(helm, release, chartPath, workerIndex)
 				defer clean()
 				if err != nil {
 					results <- &downloadResults{err: err}
@@ -874,7 +880,7 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre
 						chartify.UseHelm3(helm3),
 					)
 
-					out, err := c.Chartify(release.Name, chartification.Chart, chartify.WithChartifyOpts(chartification.Opts))
+					out, err := c.Chartify(release.Name, chartPath, chartify.WithChartifyOpts(chartification.Opts))
 					if err != nil {
 						results <- &downloadResults{err: err}
 						return
@@ -883,9 +889,14 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre
 						chartPath = out
 					}
 				} else if !forceDownload {
-					chartPath = release.Chart
+					// At this point, we are sure that either:
-				} else if pathExists(normalizeChart(st.basePath, release.Chart)) {
+					// 1. It is a local chart and we can use it in later process (helm upgrade/template/lint/etc)
-					chartPath = normalizeChart(st.basePath, release.Chart)
+					//    without any modification, or
+					// 2. It is a remote chart which can be safely handed over to helm,
+					//    because the version of Helm used in this transaction support downloading the chart instead,
+					//    and we don't need any modification to the chart
+				} else if pathExists(normalizeChart(st.basePath, chartPath)) {
+					chartPath = normalizeChart(st.basePath, chartPath)
 
 					if !skipDeps {
 						if err := helm.BuildDeps(release.Name, chartPath); err != nil {
@@ -914,7 +925,7 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre
 						fetchFlags = append(fetchFlags, "--version", release.Version)
 					}
 
-					pathElems = append(pathElems, release.Name, release.Chart, chartVersion)
+					pathElems = append(pathElems, release.Name, chartName, chartVersion)
 
 					chartPath = path.Join(pathElems...)
 
@@ -925,7 +936,7 @@ func (st *HelmState) PrepareCharts(helm helmexec.Interface, dir string, concurre
 					// only fetch chart if it is not already fetched
 					if _, err := os.Stat(chartPath); os.IsNotExist(err) {
 						fetchFlags = append(fetchFlags, "--untar", "--untardir", chartPath)
-						if err := helm.Fetch(release.Chart, fetchFlags...); err != nil {
+						if err := helm.Fetch(chartName, fetchFlags...); err != nil {
 							results <- &downloadResults{err: err}
 							return
 						}

pkg/state/state_gogetter_test.go

@@ -0,0 +1,61 @@
+package state
+
+import (
+	"fmt"
+	"github.com/google/go-cmp/cmp"
+	"github.com/roboll/helmfile/pkg/helmexec"
+	"io/ioutil"
+	"os"
+	"testing"
+)
+
+func TestGoGetter(t *testing.T) {
+	logger := helmexec.NewLogger(os.Stderr, "warn")
+
+	testcases := []struct {
+		chart, dir string
+		force      bool
+
+		out, err string
+	}{
+		{
+			chart: "raw/incubator",
+			dir:   "",
+			force: false,
+			out:   "raw/incubator",
+			err:   "",
+		},
+	}
+
+	for i, tc := range testcases {
+		t.Run(fmt.Sprintf("case %d", i), func(t *testing.T) {
+			d, err := ioutil.TempDir("", "testgogetter")
+			if err != nil {
+				panic(err)
+			}
+			defer os.RemoveAll(d)
+
+			st := &HelmState{
+				logger:   logger,
+				readFile: ioutil.ReadFile,
+				basePath: d,
+			}
+
+			out, err := st.goGetterChart(tc.chart, tc.dir, false)
+
+			if diff := cmp.Diff(tc.out, out); diff != "" {
+				t.Fatalf("Unexpected out:\n%s", diff)
+			}
+
+			var errMsg string
+
+			if err != nil {
+				errMsg = err.Error()
+			}
+
+			if diff := cmp.Diff(tc.err, errMsg); diff != "" {
+				t.Fatalf("Unexpected err:\n%s", diff)
+			}
+		})
+	}
+}