update remote fetch logic

Signed-off-by: yxxhero <aiopsclub@163.com>

docs/index.md

@@ -251,6 +251,10 @@ releases:
     # Values files used for rendering the chart
     values:
       # Value files passed via --values
+      # `go-getter`-style URLs are supported. See https://helmfile.readthedocs.io/en/latest/#loading-remote-environment-values-files 
+      - git::https://gitlab.com/org/repository-name.git@/config/values-remote.yaml?ref=main
+      - https://raw.githubusercontent.com/helmfile/testdata/main/remote-values/value.yaml
+
       - vault.yaml
       # Inline values, passed via a temporary values file and --values, so that it doesn't suffer from type issues like --set
       - address: https://vault.example.com
@@ -967,12 +971,13 @@ You can read more infos about the feature proposal [here](https://github.com/rob
 
 ### Loading remote Environment values files
 
-Since Helmfile v0.118.8, you can use `go-getter`-style URLs to refer to remote values files:
+Since Helmfile v0.118.8, you can use `go-getter`-style URLs to refer to remote values files. We use `@` to separate dir and the file path. This is a good idea borrowed from [helm-git](https://github.com/aslafy-z/helm-git) :
 
 ```yaml
 environments:
   cluster-azure-us-west:
     values:
+      - https://raw.githubusercontent.com/helmfile/testdata/main/remote-values/value.yaml # common http url
       - git::https://git.company.org/helmfiles/global/azure.yaml?ref=master
       - git::https://git.company.org/helmfiles/global/us-west.yaml?ref=master
       - git::https://gitlab.com/org/repository-name.git@/config/config.test.yaml?ref=main # Public Gilab Repo

pkg/remote/remote.go

@@ -198,7 +198,7 @@ func (r *Remote) Fetch(path string, cacheDirOpt ...string) (string, error) {
 	r.Logger.Debugf("remote> user: %s", u.User)
 	r.Logger.Debugf("remote> host: %s", u.Host)
 	r.Logger.Debugf("remote> dir: %s", u.Dir)
-	r.Logger.Debugf("remote> file: %s", u.File)
+	r.Logger.Debugf("remote> file: %s", file)
 
 	// This should be shared across variant commands, so that they can share cache for the shared imports
 	cacheBaseDir := ""
@@ -237,24 +237,26 @@ func (r *Remote) Fetch(path string, cacheDirOpt ...string) (string, error) {
 		cacheDirPath = filepath.Join(r.Home, cacheKey, u.Dir)
 	}
 
+	// origin is for judging whether target is file or directory
+	// e.g. os.CacheDir()/helmfile/https_github_com_cloudposse_helmfiles_git.ref=0.xx.0/origin
+	originDirOrFilePath := filepath.Join(cacheDirPath, "origin")
+
 	r.Logger.Debugf("remote> home: %s", r.Home)
 	r.Logger.Debugf("remote> getter dest: %s", getterDst)
 	r.Logger.Debugf("remote> cached dir: %s", cacheDirPath)
 
-	{
+	if r.fs.FileExistsAt(cacheDirPath) {
-		if r.fs.FileExistsAt(cacheDirPath) {
+		return "", fmt.Errorf("%s is not directory. please remove it so that variant could use it for dependency caching", getterDst)
-			return "", fmt.Errorf("%s is not directory. please remove it so that variant could use it for dependency caching", getterDst)
+	}
-		}
 
-		if u.Getter == "normal" {
+	if u.Getter == "normal" {
-			cachedFilePath := filepath.Join(cacheDirPath, file)
+		cachedFilePath := filepath.Join(cacheDirPath, file)
-			ok, err := r.fs.FileExists(cachedFilePath)
+		ok, err := r.fs.FileExists(cachedFilePath)
-			if err == nil && ok {
+		if err == nil && ok {
-				cached = true
-			}
-		} else if r.fs.DirectoryExistsAt(cacheDirPath) {
 			cached = true
 		}
+	} else if r.fs.DirectoryExistsAt(cacheDirPath) {
+		cached = true
 	}
 
 	if !cached {
@@ -296,7 +298,7 @@ func (r *Remote) Fetch(path string, cacheDirOpt ...string) (string, error) {
 			}
 		}
 	}
-	return filepath.Join(cacheDirPath, file), nil
+	return filepath.Join(originDirOrFilePath, file), nil
 }
 
 type Getter interface {
@@ -318,13 +320,15 @@ type HttpGetter struct {
 func (g *GoGetter) Get(wd, src, dst string) error {
 	ctx := context.Background()
 
+	opts := []getter.ClientOption{}
+
 	get := &getter.Client{
 		Ctx:     ctx,
 		Src:     src,
 		Dst:     dst,
 		Pwd:     wd,
-		Mode:    getter.ClientModeDir,
+		Mode:    getter.ClientModeAny,
-		Options: []getter.ClientOption{},
+		Options: opts,
 	}
 
 	g.Logger.Debugf("client: %+v", *get)

pkg/remote/remote_test.go

@@ -7,7 +7,9 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/stretchr/testify/require"
 
+	"github.com/helmfile/helmfile/pkg/filesystem"
 	"github.com/helmfile/helmfile/pkg/helmexec"
 	"github.com/helmfile/helmfile/pkg/testhelper"
 )
@@ -17,7 +19,7 @@ func TestRemote_HttpsGitHub(t *testing.T) {
 		CacheDir(): "",
 	}
 	cachefs := map[string]string{
-		filepath.Join(CacheDir(), "https_github_com_cloudposse_helmfiles_git.ref=0.40.0/releases/kiam.yaml"): "foo: bar",
+		filepath.Join(CacheDir(), "https_github_com_cloudposse_helmfiles_git.ref=0.40.0/origin/releases/kiam.yaml"): "foo: bar",
 	}
 
 	testcases := []struct {
@@ -45,6 +47,8 @@ func TestRemote_HttpsGitHub(t *testing.T) {
 
 				hit = false
 
+				testfs.AddFiles(cachefs)
+
 				return nil
 			}
 
@@ -70,7 +74,7 @@ func TestRemote_HttpsGitHub(t *testing.T) {
 				t.Fatalf("unexpected error: %v", err)
 			}
 
-			expectedFile := filepath.Join(CacheDir(), "https_github_com_cloudposse_helmfiles_git.ref=0.40.0/releases/kiam.yaml")
+			expectedFile := filepath.Join(CacheDir(), "https_github_com_cloudposse_helmfiles_git.ref=0.40.0/origin/releases/kiam.yaml")
 			if file != expectedFile {
 				t.Errorf("unexpected file located: %s vs expected: %s", file, expectedFile)
 			}
@@ -90,7 +94,7 @@ func TestRemote_SShGitHub(t *testing.T) {
 		CacheDir(): "",
 	}
 	cachefs := map[string]string{
-		filepath.Join(CacheDir(), "ssh_github_com_helmfile_helmfiles_git.ref=0.40.0/releases/kiam.yaml"): "foo: bar",
+		filepath.Join(CacheDir(), "ssh_github_com_cloudposse_helmfiles_git.ref=0.40.0/origin/releases/kiam.yaml"): "foo: bar",
 	}
 
 	testcases := []struct {
@@ -118,6 +122,8 @@ func TestRemote_SShGitHub(t *testing.T) {
 
 				hit = false
 
+				testfs.AddFiles(cachefs)
+
 				return nil
 			}
 
@@ -137,7 +143,7 @@ func TestRemote_SShGitHub(t *testing.T) {
 				t.Fatalf("unexpected error: %v", err)
 			}
 
-			expectedFile := filepath.Join(CacheDir(), "ssh_github_com_helmfile_helmfiles_git.ref=0.40.0/releases/kiam.yaml")
+			expectedFile := filepath.Join(CacheDir(), "ssh_github_com_cloudposse_helmfiles_git.ref=0.40.0/origin/releases/kiam.yaml")
 			if file != expectedFile {
 				t.Errorf("unexpected file located: %s vs expected: %s", file, expectedFile)
 			}
@@ -157,7 +163,7 @@ func TestRemote_SShGitHub_WithSshKey(t *testing.T) {
 		CacheDir(): "",
 	}
 	cachefs := map[string]string{
-		filepath.Join(CacheDir(), "ssh_github_com_helmfile_helmfiles_git.ref=0.40.0_sshkey=redacted/releases/kiam.yaml"): "foo: bar",
+		filepath.Join(CacheDir(), "ssh_github_com_cloudposse_helmfiles_git.ref=0.40.0_sshkey=redacted/origin/releases/kiam.yaml"): "foo: bar",
 	}
 
 	testcases := []struct {
@@ -185,6 +191,8 @@ func TestRemote_SShGitHub_WithSshKey(t *testing.T) {
 
 				hit = false
 
+				testfs.AddFiles(cachefs)
+
 				return nil
 			}
 
@@ -204,7 +212,7 @@ func TestRemote_SShGitHub_WithSshKey(t *testing.T) {
 				t.Fatalf("unexpected error: %v", err)
 			}
 
-			expectedFile := filepath.Join(CacheDir(), "ssh_github_com_helmfile_helmfiles_git.ref=0.40.0_sshkey=redacted/releases/kiam.yaml")
+			expectedFile := filepath.Join(CacheDir(), "ssh_github_com_cloudposse_helmfiles_git.ref=0.40.0_sshkey=redacted/origin/releases/kiam.yaml")
 			if file != expectedFile {
 				t.Errorf("unexpected file located: %s vs expected: %s", file, expectedFile)
 			}
@@ -297,6 +305,7 @@ func TestParse(t *testing.T) {
 		dir    string
 		file   string
 		query  string
+		user   string
 		err    string
 	}{
 		{
@@ -312,6 +321,24 @@ func TestParse(t *testing.T) {
 			dir:    "/stakater/Forecastle.git",
 			file:   "deployments/kubernetes/chart/forecastle",
 			query:  "ref=v1.0.54",
+			user:   "user:password",
+		},
+		{
+			name:   "remote path with full args",
+			input:  "git::https://user:password@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",
+			user:   "user:password",
+		},
+		{
+			name:   "remote path with no file",
+			input:  "git::https://github.com/stakater/Forecastle.git",
+			getter: "git",
+			scheme: "https",
+			dir:    "/stakater/Forecastle.git",
 		},
 		{
 			name:   "s3 scheme",
@@ -391,13 +418,14 @@ func TestParse(t *testing.T) {
 				t.Fatalf("Unexpected error:\n%s", diff)
 			}
 
-			var getter, scheme, dir, file, query string
+			var getter, scheme, dir, file, query, user string
 			if src != nil {
 				getter = src.Getter
 				scheme = src.Scheme
 				dir = src.Dir
 				file = src.File
 				query = src.RawQuery
+				user = src.User
 			}
 
 			if diff := cmp.Diff(tt.getter, getter); diff != "" {
@@ -419,6 +447,9 @@ func TestParse(t *testing.T) {
 			if diff := cmp.Diff(tt.query, query); diff != "" {
 				t.Fatalf("Unexpected query:\n%s", diff)
 			}
+			if diff := cmp.Diff(tt.user, user); diff != "" {
+				t.Fatalf("Unexpected user:\n%s", diff)
+			}
 		})
 	}
 }
@@ -436,7 +467,7 @@ func TestRemote_Fetch(t *testing.T) {
 		CacheDir(): "",
 	}
 	cachefs := map[string]string{
-		filepath.Join(CacheDir(), "https_github_com_helmfile_helmfile_git.ref=v0.151.0/README.md"): "foo: bar",
+		filepath.Join(CacheDir(), "https_github_com_helmfile_helmfile_git.ref=v0.151.0/origin/README.md"): "foo: bar",
 	}
 
 	testcases := []struct {
@@ -467,6 +498,8 @@ func TestRemote_Fetch(t *testing.T) {
 
 				hit = false
 
+				testfs.AddFiles(cachefs)
+
 				return nil
 			}
 
@@ -486,7 +519,7 @@ func TestRemote_Fetch(t *testing.T) {
 				t.Fatalf("unexpected error: %v", err)
 			}
 
-			expectedFile := filepath.Join(CacheDir(), "https_github_com_helmfile_helmfile_git.ref=v0.151.0/README.md")
+			expectedFile := filepath.Join(CacheDir(), "https_github_com_helmfile_helmfile_git.ref=v0.151.0/origin/README.md")
 			if file != expectedFile {
 				t.Errorf("unexpected file located: %s vs expected: %s", file, expectedFile)
 			}
@@ -500,3 +533,53 @@ func TestRemote_Fetch(t *testing.T) {
 		})
 	}
 }
+
+func TestRemote_CommonHttpUrl(t *testing.T) {
+	testcases := []struct {
+		name   string
+		input  string
+		rpath  string
+		errStr string
+	}{
+		{
+			name:  "common git url",
+			input: "git::https://github.com/helmfile/helmfile.git?ref=v0.153.1",
+			rpath: filepath.Join(CacheDir(), "https_github_com_helmfile_helmfile_git.ref=v0.153.1/origin"),
+		},
+		{
+			name:  "common git url with exist subpath",
+			input: "git::https://github.com/dragonflyoss/helm-charts.git@charts?ref=dragonfly-1.0.2",
+			rpath: filepath.Join(CacheDir(), "https_github_com_dragonflyoss_helm-charts_git.ref=dragonfly-1.0.2/origin/charts"),
+		},
+		{
+			name:  "common git url with no-exist subpath",
+			input: "git::https://github.com/dragonflyoss/helm-charts.git@no-existcharts?ref=dragonfly-1.0.2",
+			rpath: filepath.Join(CacheDir(), "https_github_com_dragonflyoss_helm-charts_git.ref=dragonfly-1.0.2/origin/no-existcharts"),
+		},
+		{
+			name:  "common http url",
+			input: "https://raw.githubusercontent.com/helmfile/testdata/main/remote-values/value.yaml",
+			rpath: filepath.Join(CacheDir(), "https_raw_githubusercontent_com_helmfile_testdata_main_remote-values_value_yaml/origin"),
+		},
+		{
+			name:   "common http no-exist url",
+			input:  "https://raw.githubusercontent.com/helmfile/testdata/main/remote-values/no-exist-value.yaml",
+			errStr: "get: bad response code: 404",
+		},
+	}
+
+	for _, tt := range testcases {
+		t.Run(tt.name, func(t *testing.T) {
+			remote := NewRemote(helmexec.NewLogger(io.Discard, "debug"), CacheDir(), filesystem.DefaultFileSystem())
+
+			rPath, err := remote.Fetch(tt.input)
+			errStr := ""
+			if err != nil {
+				errStr = err.Error()
+			}
+
+			require.Equalf(t, tt.errStr, errStr, "unexpected error: %s", err)
+			require.Equalf(t, tt.rpath, rPath, "unexpected rpath: %s", rPath)
+		})
+	}
+}

pkg/state/helmx.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/helmfile/chartify"
 
@@ -134,6 +135,9 @@ func (st *HelmState) goGetterChart(chart, dir, cacheDir string, force bool) (str
 		chart = dir
 	}
 
+	if strings.HasPrefix(chart, "oci://") {
+		return chart, nil
+	}
 	_, err := remote.Parse(chart)
 	if err != nil {
 		if force {

pkg/state/storage_test.go

@@ -42,7 +42,7 @@ func TestStorage_resolveFile(t *testing.T) {
 			wantErr:     false,
 		},
 		{
-			name: "non existing file in repo produce skip",
+			name: "non existing file in repo produce non skip",
 			args: args{
 				path:               "git::https://github.com/helmfile/helmfile.git@examples/values/non-existing-file.yaml?ref=v0.145.2",
 				title:              "values",
@@ -107,7 +107,7 @@ func TestStorage_resolveFile(t *testing.T) {
 				title:              "values",
 				missingFileHandler: &infoHandler,
 			},
-			wantFiles:   []string{fmt.Sprintf("%s/%s", cacheDir, "values/https_github_com_helmfile_helmfile_git.ref=v0.145.2/examples/values/replica-values.yaml")},
+			wantFiles:   []string{fmt.Sprintf("%s/%s", cacheDir, "values/https_github_com_helmfile_helmfile_git.ref=v0.145.2/origin/examples/values/replica-values.yaml")},
 			wantSkipped: false,
 			wantErr:     false,
 		},

pkg/testhelper/testfs.go

@@ -22,16 +22,21 @@ type TestFs struct {
 }
 
 func NewTestFs(files map[string]string) *TestFs {
-	dirs := map[string]bool{}
+	fsDirs := map[string]bool{}
 	for abs := range files {
-		for d := filepath.ToSlash(filepath.Dir(abs)); !dirs[d]; d = filepath.ToSlash(filepath.Dir(d)) {
+		for d := filepath.ToSlash(filepath.Dir(abs)); !fsDirs[d]; d = filepath.ToSlash(filepath.Dir(d)) {
-			dirs[d] = true
+			fsDirs[d] = true
 		}
 	}
+	fsFiles := map[string]string{}
+
+	for abs, content := range files {
+		fsFiles[filepath.ToSlash(abs)] = content
+	}
 	return &TestFs{
 		Cwd:   "/path/to",
-		dirs:  dirs,
+		dirs:  fsDirs,
-		files: files,
+		files: fsFiles,
 
 		successfulReads: []string{},
 
@@ -155,3 +160,20 @@ func (f *TestFs) Chdir(dir string) error {
 	}
 	return fmt.Errorf("unexpected chdir \"%s\"", dir)
 }
+
+func (f *TestFs) AddFiles(files map[string]string) {
+	dirs := map[string]bool{}
+	for abs := range files {
+		for d := filepath.ToSlash(filepath.Dir(abs)); !dirs[d]; d = filepath.ToSlash(filepath.Dir(d)) {
+			dirs[d] = true
+		}
+	}
+
+	for k, v := range files {
+		f.files[k] = v
+	}
+
+	for k, v := range dirs {
+		f.dirs[k] = v
+	}
+}

test/e2e/template/helmfile/testdata/snapshot/issue_473_oci_chart_url_fetch/config.yaml

@@ -1,6 +1,6 @@
 localDockerRegistry:
   enabled: true
   port: 5000
-chartifyTempDir: temp2
+chartifyTempDir: oci_chart_url_fetch
 helmfileArgs:
 - fetch