Benefit from Fetch() in downloadAll() and don't use a tmpdir

Signed-off-by: Reinaldo de Souza Jr <github@rei.nal.do>

pkg/downloader/manager.go

@@ -33,6 +33,7 @@ import (
 	"github.com/pkg/errors"
 	"sigs.k8s.io/yaml"
 
+	"helm.sh/helm/v3/internal/fileutil"
 	"helm.sh/helm/v3/internal/resolver"
 	"helm.sh/helm/v3/internal/third_party/dep/fs"
 	"helm.sh/helm/v3/internal/urlutil"
@@ -239,15 +240,15 @@ func (m *Manager) resolve(req []*chart.Dependency, repoNames map[string]string)
 // It will delete versions of the chart that exist on disk and might cause
 // a conflict.
 func (m *Manager) downloadAll(deps []*chart.Dependency) error {
+	// FIXME: This already works on a chart.Lock.Dependencies
+	// So why do we check versions constraints AGAIN?
 	repos, err := m.loadChartRepositories()
 	if err != nil {
 		return err
 	}
 
-	destPath := filepath.Join(m.ChartPath, "charts")
-	tmpPath := filepath.Join(m.ChartPath, "tmpcharts")
-
 	// Create 'charts' directory if it doesn't already exist.
+	destPath := filepath.Join(m.ChartPath, "charts")
 	if fi, err := os.Stat(destPath); err != nil {
 		if err := os.MkdirAll(destPath, 0755); err != nil {
 			return err
@@ -256,22 +257,14 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
 		return errors.Errorf("%q is not a directory", destPath)
 	}
 
-	if err := fs.RenameWithFallback(destPath, tmpPath); err != nil {
-		return errors.Wrap(err, "unable to move current charts to tmp dir")
-	}
-
-	if err := os.MkdirAll(destPath, 0755); err != nil {
-		return err
-	}
-
 	fmt.Fprintf(m.Out, "Saving %d charts\n", len(deps))
 	var saveError error
-	churls := make(map[string]struct{})
+	downloadedCharts := make([]string, 0, len(deps))
 	for _, dep := range deps {
 		// No repository means the chart is in charts directory
 		if dep.Repository == "" {
 			fmt.Fprintf(m.Out, "Dependency %s did not declare a repository. Assuming it exists in the charts directory\n", dep.Name)
-			chartPath := filepath.Join(tmpPath, dep.Name)
+			chartPath := filepath.Join(destPath, dep.Name)
 			ch, err := loader.LoadDir(chartPath)
 			if err != nil {
 				return fmt.Errorf("Unable to load chart: %v", err)
@@ -306,6 +299,13 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
 			continue
 		}
 
+		// Obtaining a remote chart involves
+		// 1. Finding the chart address for a chart version based on a repo index
+		//  a. "named" (added) repos - index is cached and also includes config, such as TLS and authentication.
+		//  b. "unnamed" (not added) repos - index could be cached
+		// 2. Fetching the chart archive to the charts cache dir
+		// 3. Copying to a target location depending on the operation
+
 		// Any failure to resolve/download a chart should fail:
 		// https://github.com/helm/helm/issues/1439
 		churl, username, password, err := m.findChartURL(dep.Name, dep.Version, dep.Repository, repos)
@@ -314,11 +314,6 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
 			break
 		}
 
-		if _, ok := churls[churl]; ok {
-			fmt.Fprintf(m.Out, "Already downloaded %s from repo %s\n", dep.Name, dep.Repository)
-			continue
-		}
-
 		fmt.Fprintf(m.Out, "Downloading %s from repo %s\n", dep.Name, dep.Repository)
 
 		dl := ChartDownloader{
@@ -334,46 +329,55 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error {
 			},
 		}
 
-		if _, _, err := dl.DownloadTo(churl, "", destPath); err != nil {
+		// Fetch and checks provenance (if needed)
+		chartDest, _, err := dl.Fetch(churl, "")
+		if err != nil {
 			saveError = errors.Wrapf(err, "could not download %s", churl)
 			break
 		}
 
-		churls[churl] = struct{}{}
+		downloadedCharts = append(downloadedCharts, chartDest)
 	}
 
-	if saveError == nil {
+	// Handle save error
-		fmt.Fprintln(m.Out, "Deleting outdated charts")
+	if saveError != nil {
-		for _, dep := range deps {
+		fmt.Fprintln(m.Out, "Save error occurred: ", saveError)
-			// Chart from local charts directory stays in place
+		fmt.Fprintln(m.Out, "Deleting newly downloaded charts, restoring pre-update state")
-			if dep.Repository != "" {
+		return saveError
-				if err := m.safeDeleteDep(dep.Name, tmpPath); err != nil {
+	}
-					return err
+
-				}
+	// No errors. All dependencies have been resolved and fetched.
-			}
+	// Delete outdated charts and copy latest versions to unpacked chart dependencies dir (./charts)
+	fmt.Fprintln(m.Out, "Deleting outdated charts")
+	for _, dep := range deps {
+		// Chart from local charts directory stays in place
+		if dep.Repository == "" {
+			continue
 		}
-		if err := move(tmpPath, destPath); err != nil {
+
+		if err := m.safeDeleteDep(dep.Name, destPath); err != nil {
 			return err
 		}
-		if err := os.RemoveAll(tmpPath); err != nil {
+	}
-			return errors.Wrapf(err, "failed to remove %v", tmpPath)
+
-		}
+	// Copying files over
-	} else {
+	for _, downloadedChart := range downloadedCharts {
-		fmt.Fprintln(m.Out, "Save error occurred: ", saveError)
+		dst := filepath.Join(destPath, filepath.Base(downloadedChart))
-		fmt.Fprintln(m.Out, "Deleting newly downloaded charts, restoring pre-update state")
+		if err := fileutil.AtomicCopyFile(downloadedChart, dst, 0644); err != nil {
-		for _, dep := range deps {
+			return err
-			if err := m.safeDeleteDep(dep.Name, destPath); err != nil {
-				return err
-			}
 		}
-		if err := os.RemoveAll(destPath); err != nil {
+
-			return errors.Wrapf(err, "failed to remove %v", destPath)
+		// TODO: confirm if there's any reason to include provenance files in charts/ for an unpacked chart.
+		chartProvenancePath := downloadedChart + provenanceFileExtension
+		if _, err := os.Stat(chartProvenancePath); os.IsNotExist(err) {
+			continue
 		}
-		if err := fs.RenameWithFallback(tmpPath, destPath); err != nil {
+
-			return errors.Wrap(err, "unable to move current charts to tmp dir")
+		if err := fileutil.AtomicCopyFile(chartProvenancePath, dst+provenanceFileExtension, 0644); err != nil {
+			return err
 		}
-		return saveError
 	}
+
 	return nil
 }
 
@@ -644,6 +648,7 @@ func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error {
 //
 // If it finds a URL that is "relative", it will prepend the repoURL.
 func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*repo.ChartRepository) (url, username, password string, err error) {
+	// Chart is available in a named repo
 	for _, cr := range repos {
 		if urlutil.Equal(repoURL, cr.Config.URL) {
 			var entry repo.ChartVersions
@@ -665,10 +670,13 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*
 			return
 		}
 	}
+
+	// Downloads repo's index and try to find the chart
 	url, err = repo.FindChartInRepoURL(repoURL, name, version, "", "", "", m.Getters)
 	if err == nil {
 		return
 	}
+
 	err = errors.Errorf("chart %s not found in %s: %s", name, repoURL, err)
 	return
 }