remove unnecessary remote.Locate() (#565)

* remove unnecessary Locate

Signed-off-by: Quan TRAN <account@itscaro.me>

* add remote test

Signed-off-by: Quan TRAN <itscaro@users.noreply.github.com>

---------

Signed-off-by: Quan TRAN <account@itscaro.me>
Signed-off-by: Quan TRAN <itscaro@users.noreply.github.com>
Co-authored-by: yxxhero <11087727+yxxhero@users.noreply.github.com>

pkg/app/app.go

@@ -1140,7 +1140,7 @@ func (a *App) WrapWithoutSelector(converge func(*state.HelmState, helmexec.Inter
 }
 
 func (a *App) findDesiredStateFiles(specifiedPath string, opts LoadOpts) ([]string, error) {
-	path, err := a.remote.Locate(specifiedPath)
+	path, err := a.remote.Locate(specifiedPath, "states")
 	if err != nil {
 		return nil, fmt.Errorf("locate: %v", err)
 	}

pkg/app/desired_state_file_loader.go

@@ -101,7 +101,7 @@ func (ld *desiredStateLoader) Load(f string, opts LoadOpts) (*state.HelmState, e
 }
 
 func (ld *desiredStateLoader) loadFile(inheritedEnv, overrodeEnv *environment.Environment, baseDir, file string, evaluateBases bool) (*state.HelmState, error) {
-	path, err := ld.remote.Locate(file)
+	path, err := ld.remote.Locate(file, "states")
 	if err != nil {
 		return nil, fmt.Errorf("locate: %v", err)
 	}

pkg/remote/remote.go

@@ -54,11 +54,11 @@ type Remote struct {
 // Locate takes an URL to a remote file or a path to a local file.
 // If the argument was an URL, it fetches the remote directory contained within the URL,
 // and returns the path to the file in the fetched directory
-func (r *Remote) Locate(urlOrPath string) (string, error) {
+func (r *Remote) Locate(urlOrPath string, cacheDirOpt ...string) (string, error) {
 	if r.fs.FileExistsAt(urlOrPath) || r.fs.DirectoryExistsAt(urlOrPath) {
 		return urlOrPath, nil
 	}
-	fetched, err := r.Fetch(urlOrPath)
+	fetched, err := r.Fetch(urlOrPath, cacheDirOpt...)
 	if err != nil {
 		if _, ok := err.(InvalidURLError); ok {
 			return urlOrPath, nil

pkg/remote/remote_test.go

@@ -304,3 +304,76 @@ type testGetter struct {
 func (t *testGetter) Get(wd, src, dst string) error {
 	return t.get(wd, src, dst)
 }
+
+func TestRemote_Fetch(t *testing.T) {
+	cleanfs := map[string]string{
+		CacheDir(): "",
+	}
+	cachefs := map[string]string{
+		filepath.Join(CacheDir(), "https_github_com_helmfile_helmfile_git.ref=v0.151.0/README.md"): "foo: bar",
+	}
+
+	type testcase struct {
+		files          map[string]string
+		expectCacheHit bool
+		cacheDirOpt    string
+	}
+
+	testcases := []testcase{
+		{files: cleanfs, expectCacheHit: false, cacheDirOpt: ""},
+		{files: cachefs, expectCacheHit: true, cacheDirOpt: ""},
+		{files: cleanfs, expectCacheHit: false, cacheDirOpt: "states"},
+		{files: cachefs, expectCacheHit: true, cacheDirOpt: "states"},
+	}
+
+	for i := range testcases {
+		testcase := testcases[i]
+
+		t.Run(fmt.Sprintf("case %d", i), func(t *testing.T) {
+			testfs := testhelper.NewTestFs(testcase.files)
+
+			hit := true
+
+			get := func(wd, src, dst string) error {
+				if wd != CacheDir() {
+					return fmt.Errorf("unexpected wd: %s", wd)
+				}
+				if src != "git::https://github.com/helmfile/helmfile.git?ref=v0.151.0" {
+					return fmt.Errorf("unexpected src: %s", src)
+				}
+
+				hit = false
+
+				return nil
+			}
+
+			getter := &testGetter{
+				get: get,
+			}
+			remote := &Remote{
+				Logger: helmexec.NewLogger(io.Discard, "debug"),
+				Home:   CacheDir(),
+				Getter: getter,
+				fs:     testfs.ToFileSystem(),
+			}
+
+			url := "git::https://github.com/helmfile/helmfile.git@README.md?ref=v0.151.0"
+			file, err := remote.Fetch(url)
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+
+			expectedFile := filepath.Join(CacheDir(), "https_github_com_helmfile_helmfile_git.ref=v0.151.0/README.md")
+			if file != expectedFile {
+				t.Errorf("unexpected file located: %s vs expected: %s", file, expectedFile)
+			}
+
+			if testcase.expectCacheHit && !hit {
+				t.Errorf("unexpected result: unexpected cache miss")
+			}
+			if !testcase.expectCacheHit && hit {
+				t.Errorf("unexpected result: unexpected cache hit")
+			}
+		})
+	}
+}

pkg/state/create.go

@@ -303,15 +303,9 @@ func (c *StateCreator) scatterGatherEnvSecretFiles(st *HelmState, envSecretFiles
 		},
 		func(id int) {
 			for secret := range secrets {
-				urlOrPath := secret.path
-				localPath, err := c.remote.Locate(urlOrPath)
-				if err == nil {
-					urlOrPath = localPath
-				}
-
 				release := &ReleaseSpec{}
 				flags := st.appendConnectionFlags([]string{}, release)
-				decFile, err := helm.DecryptSecret(st.createHelmContext(release, 0), urlOrPath, flags...)
+				decFile, err := helm.DecryptSecret(st.createHelmContext(release, 0), secret.path, flags...)
 				if err != nil {
 					results <- secretResult{secret.id, nil, err, secret.path}
 					continue

pkg/state/envvals_loader.go

@@ -42,13 +42,7 @@ func (ld *EnvironmentValuesLoader) LoadEnvironmentValues(missingFileHandler *str
 
 		switch strOrMap := entry.(type) {
 		case string:
-			urlOrPath := strOrMap
+			files, skipped, err := ld.storage.resolveFile(missingFileHandler, "environment values", entry.(string))
-			localPath, err := ld.remote.Locate(urlOrPath)
-			if err == nil {
-				urlOrPath = localPath
-			}
-
-			files, skipped, err := ld.storage.resolveFile(missingFileHandler, "environment values", urlOrPath)
 			if err != nil {
 				return nil, err
 			}