fix: missing Helm params (#9565) (#9566)

* fix: missing Helm params Signed-off-by: Michael Crenshaw <michael@crenshaw.dev> * use absolute paths, fix tests Signed-off-by: Michael Crenshaw <michael@crenshaw.dev> * fix race in test Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
argoproj · Jun 5, 2022 · a6c5874 · a6c5874
1 parent 46af59e
commit a6c5874
Show file tree

Hide file tree

Showing 11 changed files with 77 additions and 16 deletions.
diff --git a/reposerver/repository/repository_test.go b/reposerver/repository/repository_test.go
@@ -1129,6 +1129,7 @@ func TestListApps(t *testing.T) {
 		"kustomization_yml":              "Kustomize",
 		"my-chart":                       "Helm",
 		"my-chart-2":                     "Helm",
+		"values-files":                   "Helm",
 	}
 	assert.Equal(t, expectedApps, res.Apps)
 }
@@ -1874,3 +1875,32 @@ func runGit(t *testing.T, workDir string, args ...string) string {
 	require.NoError(t, err, stringOut)
 	return stringOut
 }
+
+func Test_findHelmValueFilesInPath(t *testing.T) {
+	t.Run("does not exist", func(t *testing.T) {
+		files, err := findHelmValueFilesInPath("/obviously/does/not/exist")
+		assert.Error(t, err)
+		assert.Empty(t, files)
+	})
+	t.Run("values files", func(t *testing.T) {
+		files, err := findHelmValueFilesInPath("./testdata/values-files")
+		assert.NoError(t, err)
+		assert.Len(t, files, 4)
+	})
+}
+
+func Test_populateHelmAppDetails(t *testing.T) {
+	res := apiclient.RepoAppDetailsResponse{}
+	q := apiclient.RepoServerAppDetailsQuery{
+		Repo: &argoappv1.Repository{},
+		Source: &argoappv1.ApplicationSource{
+			Helm: &argoappv1.ApplicationSourceHelm{ValueFiles: []string{"exclude.yaml", "has-the-word-values.yaml"}},
+		},
+	}
+	appPath, err := filepath.Abs("./testdata/values-files/")
+	require.NoError(t, err)
+	err = populateHelmAppDetails(&res, appPath, appPath, &q)
+	require.NoError(t, err)
+	assert.Len(t, res.Helm.Parameters, 3)
+	assert.Len(t, res.Helm.ValueFiles, 4)
+}
diff --git a/reposerver/repository/testdata/values-files/Chart.yaml b/reposerver/repository/testdata/values-files/Chart.yaml
@@ -0,0 +1,2 @@
+name: my-chart
+version: 1.1.0
diff --git a/reposerver/repository/testdata/values-files/caps-extn-values.YAML b/reposerver/repository/testdata/values-files/caps-extn-values.YAML
diff --git a/reposerver/repository/testdata/values-files/exclude.yaml b/reposerver/repository/testdata/values-files/exclude.yaml
@@ -0,0 +1 @@
+exclude: yaml
diff --git a/reposerver/repository/testdata/values-files/has-the-word-values.yaml b/reposerver/repository/testdata/values-files/has-the-word-values.yaml
@@ -0,0 +1,4 @@
+has:
+  the:
+    word:
+      values: yaml
diff --git a/reposerver/repository/testdata/values-files/values.yaml b/reposerver/repository/testdata/values-files/values.yaml
@@ -0,0 +1 @@
+values: yaml
diff --git a/reposerver/repository/testdata/values-files/values.yml b/reposerver/repository/testdata/values-files/values.yml
diff --git a/server/server_test.go b/server/server_test.go
@@ -799,14 +799,15 @@ func TestAuthenticate_bad_request_metadata(t *testing.T) {
 		},
 	}
 
-	ctx := context.Background()
-
 	for _, testData := range tests {
 		testDataCopy := testData
 
 		t.Run(testDataCopy.test, func(t *testing.T) {
 			t.Parallel()
 
+			// Must be declared here to avoid race.
+			ctx := context.Background()  //nolint:ineffassign,staticcheck
+
 			argocd, _ := getTestServer(t, testDataCopy.anonymousEnabled, true)
 			ctx = metadata.NewIncomingContext(context.Background(), testDataCopy.metadata)
 

diff --git a/util/helm/helm.go b/util/helm/helm.go
@@ -6,7 +6,6 @@ import (
 	"net/url"
 	"os"
 	"os/exec"
-	"path"
 	"strings"
 
 	"github.com/ghodss/yaml"
@@ -143,11 +142,10 @@ func (h *helm) GetParameters(valuesFiles []pathutil.ResolvedFilePath) (map[strin
 		if err == nil && (parsedURL.Scheme == "http" || parsedURL.Scheme == "https") {
 			fileValues, err = config.ReadRemoteFile(file)
 		} else {
-			filePath := path.Join(h.cmd.WorkDir, file)
-			if _, err := os.Stat(filePath); os.IsNotExist(err) {
+			if _, err := os.Stat(file); os.IsNotExist(err) {
 				continue
 			}
-			fileValues, err = ioutil.ReadFile(filePath)
+			fileValues, err = ioutil.ReadFile(file)
 		}
 		if err != nil {
 			return nil, fmt.Errorf("failed to read value file %s: %s", file, err)

diff --git a/util/helm/helm_test.go b/util/helm/helm_test.go
@@ -1,8 +1,11 @@
 package helm
 
 import (
+	"path/filepath"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"github.com/argoproj/argo-cd/v2/util/io/path"
 
 	"github.com/argoproj/gitops-engine/pkg/utils/kube"
@@ -51,11 +54,16 @@ func TestHelmTemplateParams(t *testing.T) {
 }
 
 func TestHelmTemplateValues(t *testing.T) {
-	h, err := NewHelmApp("./testdata/redis", []HelmRepository{}, false, "", "", false)
+	repoRoot := "./testdata/redis"
+	repoRootAbs, err := filepath.Abs(repoRoot)
+	require.NoError(t, err)
+	h, err := NewHelmApp(repoRootAbs, []HelmRepository{}, false, "", "", false)
 	assert.NoError(t, err)
+	valuesPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-production.yaml", nil)
+	require.NoError(t, err)
 	opts := TemplateOpts{
 		Name:   "test",
-		Values: []path.ResolvedFilePath{"values-production.yaml"},
+		Values: []path.ResolvedFilePath{valuesPath},
 	}
 	objs, err := template(h, &opts)
 	assert.Nil(t, err)
@@ -72,33 +80,48 @@ func TestHelmTemplateValues(t *testing.T) {
 }
 
 func TestHelmGetParams(t *testing.T) {
-	h, err := NewHelmApp("./testdata/redis", nil, false, "", "", false)
+	repoRoot := "./testdata/redis"
+	repoRootAbs, err := filepath.Abs(repoRoot)
+	require.NoError(t, err)
+	h, err := NewHelmApp(repoRootAbs, nil, false, "", "", false)
 	assert.NoError(t, err)
 	params, err := h.GetParameters(nil)
 	assert.Nil(t, err)
 
 	slaveCountParam := params["cluster.slaveCount"]
-	assert.Equal(t, slaveCountParam, "1")
+	assert.Equal(t, "1", slaveCountParam)
 }
 
 func TestHelmGetParamsValueFiles(t *testing.T) {
-	h, err := NewHelmApp("./testdata/redis", nil, false, "", "", false)
+	repoRoot := "./testdata/redis"
+	repoRootAbs, err := filepath.Abs(repoRoot)
+	require.NoError(t, err)
+	h, err := NewHelmApp(repoRootAbs, nil, false, "", "", false)
 	assert.NoError(t, err)
-	params, err := h.GetParameters([]path.ResolvedFilePath{"values-production.yaml"})
+	valuesPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-production.yaml", nil)
+	require.NoError(t, err)
+	params, err := h.GetParameters([]path.ResolvedFilePath{valuesPath})
 	assert.Nil(t, err)
 
 	slaveCountParam := params["cluster.slaveCount"]
-	assert.Equal(t, slaveCountParam, "3")
+	assert.Equal(t, "3", slaveCountParam)
 }
 
 func TestHelmGetParamsValueFilesThatExist(t *testing.T) {
-	h, err := NewHelmApp("./testdata/redis", nil, false, "", "", false)
+	repoRoot := "./testdata/redis"
+	repoRootAbs, err := filepath.Abs(repoRoot)
+	require.NoError(t, err)
+	h, err := NewHelmApp(repoRootAbs, nil, false, "", "", false)
 	assert.NoError(t, err)
-	params, err := h.GetParameters([]path.ResolvedFilePath{"values-missing.yaml", "values-production.yaml"})
+	valuesMissingPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-missing.yaml", nil)
+	require.NoError(t, err)
+	valuesProductionPath, _, err := path.ResolveFilePath(repoRootAbs, repoRootAbs, "values-production.yaml", nil)
+	require.NoError(t, err)
+	params, err := h.GetParameters([]path.ResolvedFilePath{valuesMissingPath, valuesProductionPath})
 	assert.Nil(t, err)
 
 	slaveCountParam := params["cluster.slaveCount"]
-	assert.Equal(t, slaveCountParam, "3")
+	assert.Equal(t, "3", slaveCountParam)
 }
 
 func TestHelmTemplateReleaseNameOverwrite(t *testing.T) {

diff --git a/util/io/path/resolved.go b/util/io/path/resolved.go
@@ -11,6 +11,7 @@ import (
 )
 
 // ResolvedFilePath represents a resolved file path and intended to prevent unintentional use of not verified file path.
+// It is always either a URL or an absolute path.
 type ResolvedFilePath string
 
 // resolveSymbolicLinkRecursive resolves the symlink path recursively to its