From b35ab1aa208aa2d9d79a0fc33bcede9328c484cf Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov Date: Sat, 10 Aug 2024 08:20:24 +0300 Subject: [PATCH 1/4] feat(dependency): Add --skip-download-if-exists into dependency build/update Signed-off-by: Suleiman Dibirov --- pkg/action/dependency.go | 1 + pkg/cmd/dependency.go | 1 + pkg/cmd/dependency_build.go | 21 ++++--- pkg/cmd/dependency_update.go | 21 ++++--- pkg/downloader/chart_downloader.go | 82 +++++++++++++++++++++---- pkg/downloader/manager.go | 32 ++++++++-- pkg/downloader/manager_test.go | 99 ++++++++++++++++++++++++++++++ 7 files changed, 218 insertions(+), 39 deletions(-) diff --git a/pkg/action/dependency.go b/pkg/action/dependency.go index 03c370c8e..82b69b676 100644 --- a/pkg/action/dependency.go +++ b/pkg/action/dependency.go @@ -37,6 +37,7 @@ type Dependency struct { Verify bool Keyring string SkipRefresh bool + SkipDownloadIfExists bool ColumnWidth uint Username string Password string diff --git a/pkg/cmd/dependency.go b/pkg/cmd/dependency.go index 34bbff6be..587b16dc7 100644 --- a/pkg/cmd/dependency.go +++ b/pkg/cmd/dependency.go @@ -133,4 +133,5 @@ func addDependencySubcommandFlags(f *pflag.FlagSet, client *action.Dependency) { f.BoolVar(&client.InsecureSkipTLSverify, "insecure-skip-tls-verify", false, "skip tls certificate checks for the chart download") f.BoolVar(&client.PlainHTTP, "plain-http", false, "use insecure HTTP connections for the chart download") f.StringVar(&client.CaFile, "ca-file", "", "verify certificates of HTTPS-enabled servers using this CA bundle") + f.BoolVar(&client.SkipDownloadIfExists, "skip-download-if-exists", false, "skip download of the chart if it already exists in the charts directory") } diff --git a/pkg/cmd/dependency_build.go b/pkg/cmd/dependency_build.go index 320fe12ae..5946e76b6 100644 --- a/pkg/cmd/dependency_build.go +++ b/pkg/cmd/dependency_build.go @@ -61,16 +61,17 @@ func newDependencyBuildCmd(out io.Writer) *cobra.Command { } man := &downloader.Manager{ - Out: out, - ChartPath: chartpath, - Keyring: client.Keyring, - SkipUpdate: client.SkipRefresh, - Getters: getter.All(settings), - RegistryClient: registryClient, - RepositoryConfig: settings.RepositoryConfig, - RepositoryCache: settings.RepositoryCache, - ContentCache: settings.ContentCache, - Debug: settings.Debug, + Out: out, + ChartPath: chartpath, + Keyring: client.Keyring, + SkipUpdate: client.SkipRefresh, + SkipDownloadIfExists: client.SkipDownloadIfExists, + Getters: getter.All(settings), + RegistryClient: registryClient, + RepositoryConfig: settings.RepositoryConfig, + RepositoryCache: settings.RepositoryCache, + ContentCache: settings.ContentCache, + Debug: settings.Debug, } if client.Verify { man.Verify = downloader.VerifyIfPossible diff --git a/pkg/cmd/dependency_update.go b/pkg/cmd/dependency_update.go index b534fb48a..346d8253d 100644 --- a/pkg/cmd/dependency_update.go +++ b/pkg/cmd/dependency_update.go @@ -65,16 +65,17 @@ func newDependencyUpdateCmd(_ *action.Configuration, out io.Writer) *cobra.Comma } man := &downloader.Manager{ - Out: out, - ChartPath: chartpath, - Keyring: client.Keyring, - SkipUpdate: client.SkipRefresh, - Getters: getter.All(settings), - RegistryClient: registryClient, - RepositoryConfig: settings.RepositoryConfig, - RepositoryCache: settings.RepositoryCache, - ContentCache: settings.ContentCache, - Debug: settings.Debug, + Out: out, + ChartPath: chartpath, + Keyring: client.Keyring, + SkipUpdate: client.SkipRefresh, + SkipDownloadIfExists: client.SkipDownloadIfExists, + Getters: getter.All(settings), + RegistryClient: registryClient, + RepositoryConfig: settings.RepositoryConfig, + RepositoryCache: settings.RepositoryCache, + ContentCache: settings.ContentCache, + Debug: settings.Debug, } if client.Verify { man.Verify = downloader.VerifyAlways diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 00c8c56e8..b8d4ba00a 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -29,6 +29,8 @@ import ( "path/filepath" "strings" + "github.com/Masterminds/semver/v3" + "helm.sh/helm/v4/internal/fileutil" ifs "helm.sh/helm/v4/internal/third_party/dep/fs" "helm.sh/helm/v4/internal/urlutil" @@ -149,12 +151,7 @@ func (c *ChartDownloader) DownloadTo(ref, version, dest string) (string, *proven } } - name := filepath.Base(u.Path) - if u.Scheme == registry.OCIScheme { - idx := strings.LastIndexByte(name, ':') - name = fmt.Sprintf("%s-%s.tgz", name[:idx], name[idx+1:]) - } - + name := c.getChartName(u.String()) destfile := filepath.Join(dest, name) if err := fileutil.AtomicWriteFile(destfile, data, 0644); err != nil { return destfile, nil, err @@ -302,11 +299,7 @@ func (c *ChartDownloader) DownloadToCache(ref, version string) (string, *provena // Note, this does make an assumption that the name/version is unique to a // hash when a provenance file is used. If this isn't true, this section of code // will need to be reworked. - name := filepath.Base(u.Path) - if u.Scheme == registry.OCIScheme { - idx := strings.LastIndexByte(name, ':') - name = fmt.Sprintf("%s-%s.tgz", name[:idx], name[idx+1:]) - } + name := c.getChartName(u.String()) // Copy chart to a known location with the right name for verification and then // clean it up. @@ -353,9 +346,9 @@ func (c *ChartDownloader) DownloadToCache(ref, version string) (string, *provena // // TODO: support OCI hash func (c *ChartDownloader) ResolveChartVersion(ref, version string) (string, *url.URL, error) { - u, err := url.Parse(ref) + u, err := c.parseChartURL(ref, version) if err != nil { - return "", nil, fmt.Errorf("invalid chart URL format: %s", ref) + return "", u, err } if registry.IsOCI(u.String()) { @@ -574,6 +567,69 @@ func (c *ChartDownloader) scanReposForURL(u string, rf *repo.File) (*repo.Entry, return nil, ErrNoOwnerRepo } +func (c *ChartDownloader) getChartName(url string) string { + name := filepath.Base(url) + if registry.IsOCI(url) { + idx := strings.LastIndexByte(name, ':') + name = fmt.Sprintf("%s-%s.tgz", name[:idx], name[idx+1:]) + } + + return name +} + +func (c *ChartDownloader) parseChartURL(ref string, version string) (*url.URL, error) { + u, err := url.Parse(ref) + if err != nil { + return nil, fmt.Errorf("invalid chart URL format: %s", ref) + } + + if registry.IsOCI(u.String()) { + refAlreadyHasTagOrDigest := strings.Contains(u.Path, ":") || strings.Contains(u.Path, "@") + if refAlreadyHasTagOrDigest { + return u, nil + } + + tag, err := c.getOciTag(ref, version) + if err != nil { + return nil, err + } + + u.Path = fmt.Sprintf("%s:%s", u.Path, tag) + } + + return u, nil +} + +func (c *ChartDownloader) getOciTag(ref, version string) (string, error) { + var tag string + + // Evaluate whether an explicit version has been provided. Otherwise, determine version to use + _, errSemVer := semver.NewVersion(version) + if errSemVer == nil { + tag = version + } else { + // Retrieve list of repository tags + tags, err := c.RegistryClient.Tags(strings.TrimPrefix(ref, fmt.Sprintf("%s://", registry.OCIScheme))) + if err != nil { + return "", err + } + if len(tags) == 0 { + return "", fmt.Errorf("unable to locate any tags in provided repository: %s", ref) + } + + // Determine if version provided + // If empty, try to get the highest available tag + // If exact version, try to find it + // If semver constraint string, try to find a match + tag, err = registry.GetTagMatchingVersionOrConstraint(tags, version) + if err != nil { + return "", err + } + } + + return tag, nil +} + func loadRepoConfig(file string) (*repo.File, error) { r, err := repo.LoadFile(file) if err != nil && !errors.Is(err, fs.ErrNotExist) { diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index d41b8fdb4..34c696c6a 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -27,6 +27,7 @@ import ( "os" "path/filepath" "regexp" + "slices" "strings" "sync" @@ -70,6 +71,8 @@ type Manager struct { Keyring string // SkipUpdate indicates that the repository should not be updated first. SkipUpdate bool + // SkipDownloadIfExists indicates that the chart should not be downloaded if it already exists. + SkipDownloadIfExists bool // Getter collection for the operation Getters []getter.Provider RegistryClient *registry.Client @@ -273,6 +276,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { fmt.Fprintf(m.Out, "Saving %d charts\n", len(deps)) var saveError error churls := make(map[string]struct{}) + skippedCharts := make([]string, 0) for _, dep := range deps { // No repository means the chart is in charts directory if dep.Repository == "" { @@ -326,8 +330,6 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { continue } - fmt.Fprintf(m.Out, "Downloading %s from repo %s\n", dep.Name, dep.Repository) - dl := ChartDownloader{ Out: m.Out, Verify: m.Verify, @@ -356,6 +358,23 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { getter.WithTagName(version)) } + if m.SkipDownloadIfExists { + u, err := dl.parseChartURL(churl, version) + + if err != nil { + return err + } + + name := dl.getChartName(u.String()) + if _, err = os.Stat(filepath.Join(destPath, name)); err == nil { + fmt.Fprintf(m.Out, "Already exists locally %s from repo %s\n", dep.Name, dep.Repository) + + skippedCharts = append(skippedCharts, name) + continue + } + } + + fmt.Fprintf(m.Out, "Downloading %s from repo %s\n", dep.Name, dep.Repository) if _, _, err = dl.DownloadTo(churl, version, tmpPath); err != nil { saveError = fmt.Errorf("could not download %s: %w", churl, err) break @@ -367,7 +386,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { // TODO: this should probably be refactored to be a []error, so we can capture and provide more information rather than "last error wins". if saveError == nil { // now we can move all downloaded charts to destPath and delete outdated dependencies - if err := m.safeMoveDeps(deps, tmpPath, destPath); err != nil { + if err := m.safeMoveDeps(deps, tmpPath, destPath, skippedCharts); err != nil { return err } } else { @@ -394,13 +413,14 @@ func parseOCIRef(chartRef string) (string, string, error) { // It does this by first matching the file name to an expected pattern, then loading // the file to verify that it is a chart. // -// Any charts in dest that do not exist in source are removed (barring local dependencies) +// Any charts in dest that do not exist in source and not in skippedCharts are removed (barring local dependencies) +// skippedCharts is a list of charts that were skipped during the download process. // // Because it requires tar file introspection, it is more intensive than a basic move. // // This will only return errors that should stop processing entirely. Other errors // will emit log messages or be ignored. -func (m *Manager) safeMoveDeps(deps []*chart.Dependency, source, dest string) error { +func (m *Manager) safeMoveDeps(deps []*chart.Dependency, source, dest string, skippedCharts []string) error { existsInSourceDirectory := map[string]bool{} isLocalDependency := map[string]bool{} sourceFiles, err := os.ReadDir(source) @@ -441,7 +461,7 @@ func (m *Manager) safeMoveDeps(deps []*chart.Dependency, source, dest string) er fmt.Fprintln(m.Out, "Deleting outdated charts") // find all files that exist in dest that do not exist in source; delete them (outdated dependencies) for _, file := range destFiles { - if !file.IsDir() && !existsInSourceDirectory[file.Name()] { + if !file.IsDir() && !existsInSourceDirectory[file.Name()] && !slices.Contains(skippedCharts, file.Name()) { fname := filepath.Join(dest, file.Name()) ch, err := loader.LoadFile(fname) if err != nil { diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index 9e27f183f..c90fecf1e 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -302,6 +302,105 @@ version: 0.1.0` } } +func TestDownloadAll_SkipDownloadIfExists(t *testing.T) { + // Set up a fake repo + srv := repotest.NewTempServer( + t, + repotest.WithChartSourceGlob("testdata/*.tgz*"), + ) + defer srv.Stop() + if err := srv.LinkIndices(); err != nil { + t.Fatal(err) + } + + chartPath := t.TempDir() + contentCache := t.TempDir() + b := bytes.NewBuffer(nil) + g := getter.Providers{getter.Provider{ + Schemes: []string{"http", "https"}, + New: getter.NewHTTPGetter, + }} + m := &Manager{ + Out: b, + RepositoryConfig: filepath.Join(srv.Root(), "repositories.yaml"), + RepositoryCache: srv.Root(), + ChartPath: chartPath, + Getters: g, + ContentCache: contentCache, + } + + dep := &chart.Dependency{ + Name: "local-subchart", + Repository: srv.URL(), + Version: "0.1.0", + } + + // First download without SkipDownloadIfExists flag - should download the chart + if err := m.downloadAll([]*chart.Dependency{dep}); err != nil { + t.Fatal(err) + } + + chartFile := filepath.Join(chartPath, "charts", "local-subchart-0.1.0.tgz") + if _, err := os.Stat(chartFile); errors.Is(err, fs.ErrNotExist) { + t.Fatal("chart should have been downloaded") + } + + // Get the file info to check modification time later + fileInfo1, err := os.Stat(chartFile) + if err != nil { + t.Fatal(err) + } + + // Second download with SkipDownloadIfExists flag - should NOT re-download + m.SkipDownloadIfExists = true + b.Reset() + if err := m.downloadAll([]*chart.Dependency{dep}); err != nil { + t.Fatal(err) + } + + // Verify the chart file still exists + if _, err := os.Stat(chartFile); errors.Is(err, fs.ErrNotExist) { + t.Fatal("chart should still exist") + } + + // Verify the file was NOT re-downloaded by checking modification time hasn't changed + fileInfo2, err := os.Stat(chartFile) + if err != nil { + t.Fatal(err) + } + + if !fileInfo1.ModTime().Equal(fileInfo2.ModTime()) { + t.Errorf("chart file should not have been re-downloaded, but modification time changed: %v vs %v", fileInfo1.ModTime(), fileInfo2.ModTime()) + } + + output := b.String() + if !bytes.Contains([]byte(output), []byte("Already exists locally")) { + t.Errorf("expected 'Already exists locally' message in output, got: %s", output) + } + + // Third test: Delete the file and verify it gets re-downloaded when flag is disabled + if err := os.Remove(chartFile); err != nil { + t.Fatal(err) + } + + m.SkipDownloadIfExists = false + b.Reset() + if err := m.downloadAll([]*chart.Dependency{dep}); err != nil { + t.Fatal(err) + } + + // Verify the file was re-downloaded + if _, err := os.Stat(chartFile); errors.Is(err, fs.ErrNotExist) { + t.Fatal("chart should have been re-downloaded") + } + + // Verify the output does NOT contain "Already exists locally" + output = b.String() + if bytes.Contains([]byte(output), []byte("Already exists locally")) { + t.Error("should not skip download when file doesn't exist") + } +} + func TestUpdateBeforeBuild(t *testing.T) { // Set up a fake repo srv := repotest.NewTempServer( From c17302824d8bdab1951eac06839edd85c01f868e Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov Date: Mon, 27 Oct 2025 18:49:52 +0200 Subject: [PATCH 2/4] improve chart URL parsing and tag handling Signed-off-by: Suleiman Dibirov --- pkg/downloader/chart_downloader.go | 38 ++++++++++++++++-------------- pkg/downloader/manager.go | 6 ++++- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index b8d4ba00a..3df704ac2 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -346,9 +346,14 @@ func (c *ChartDownloader) DownloadToCache(ref, version string) (string, *provena // // TODO: support OCI hash func (c *ChartDownloader) ResolveChartVersion(ref, version string) (string, *url.URL, error) { - u, err := c.parseChartURL(ref, version) + u, err := url.Parse(ref) if err != nil { - return "", u, err + return "", nil, err + } + + u, err = c.appendTagToUrlIfNeeded(u, version) + if err != nil { + return "", nil, err } if registry.IsOCI(u.String()) { @@ -577,27 +582,24 @@ func (c *ChartDownloader) getChartName(url string) string { return name } -func (c *ChartDownloader) parseChartURL(ref string, version string) (*url.URL, error) { - u, err := url.Parse(ref) - if err != nil { - return nil, fmt.Errorf("invalid chart URL format: %s", ref) +func (c *ChartDownloader) appendTagToUrlIfNeeded(chartUrl *url.URL, version string) (*url.URL, error) { + if !registry.IsOCI(chartUrl.String()) { + return chartUrl, nil } - if registry.IsOCI(u.String()) { - refAlreadyHasTagOrDigest := strings.Contains(u.Path, ":") || strings.Contains(u.Path, "@") - if refAlreadyHasTagOrDigest { - return u, nil - } - - tag, err := c.getOciTag(ref, version) - if err != nil { - return nil, err - } + refAlreadyHasTagOrDigest := strings.Contains(chartUrl.Path, ":") || strings.Contains(chartUrl.Path, "@") + if refAlreadyHasTagOrDigest { + return chartUrl, nil + } - u.Path = fmt.Sprintf("%s:%s", u.Path, tag) + tag, err := c.getOciTag(chartUrl.String(), version) + if err != nil { + return nil, err } - return u, nil + chartUrl.Path = fmt.Sprintf("%s:%s", chartUrl.Path, tag) + + return chartUrl, nil } func (c *ChartDownloader) getOciTag(ref, version string) (string, error) { diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 34c696c6a..026a9e487 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -359,8 +359,12 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { } if m.SkipDownloadIfExists { - u, err := dl.parseChartURL(churl, version) + u, err := url.Parse(churl) + if err != nil { + return err + } + u, err = dl.appendTagToUrlIfNeeded(u, version) if err != nil { return err } From b4a70d703fb948b1002dd3f22ee768be1ff8b7c1 Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov Date: Wed, 29 Oct 2025 21:20:28 +0200 Subject: [PATCH 3/4] lint fixes Signed-off-by: Suleiman Dibirov --- pkg/downloader/chart_downloader.go | 4 ++-- pkg/downloader/manager.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 3df704ac2..79673ebb1 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -351,7 +351,7 @@ func (c *ChartDownloader) ResolveChartVersion(ref, version string) (string, *url return "", nil, err } - u, err = c.appendTagToUrlIfNeeded(u, version) + u, err = c.appendTagToURLIfNeeded(u, version) if err != nil { return "", nil, err } @@ -582,7 +582,7 @@ func (c *ChartDownloader) getChartName(url string) string { return name } -func (c *ChartDownloader) appendTagToUrlIfNeeded(chartUrl *url.URL, version string) (*url.URL, error) { +func (c *ChartDownloader) appendTagToURLIfNeeded(chartUrl *url.URL, version string) (*url.URL, error) { if !registry.IsOCI(chartUrl.String()) { return chartUrl, nil } diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 026a9e487..2411c3aad 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -364,7 +364,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { return err } - u, err = dl.appendTagToUrlIfNeeded(u, version) + u, err = dl.appendTagToURLIfNeeded(u, version) if err != nil { return err } From ea88181d34f01c83c3700f9a8114f227e0fd68d8 Mon Sep 17 00:00:00 2001 From: Suleiman Dibirov Date: Wed, 29 Oct 2025 21:39:10 +0200 Subject: [PATCH 4/4] lint fixes 2 Signed-off-by: Suleiman Dibirov --- pkg/downloader/chart_downloader.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/pkg/downloader/chart_downloader.go b/pkg/downloader/chart_downloader.go index 79673ebb1..81cdf0c76 100644 --- a/pkg/downloader/chart_downloader.go +++ b/pkg/downloader/chart_downloader.go @@ -582,24 +582,24 @@ func (c *ChartDownloader) getChartName(url string) string { return name } -func (c *ChartDownloader) appendTagToURLIfNeeded(chartUrl *url.URL, version string) (*url.URL, error) { - if !registry.IsOCI(chartUrl.String()) { - return chartUrl, nil +func (c *ChartDownloader) appendTagToURLIfNeeded(chartURL *url.URL, version string) (*url.URL, error) { + if !registry.IsOCI(chartURL.String()) { + return chartURL, nil } - refAlreadyHasTagOrDigest := strings.Contains(chartUrl.Path, ":") || strings.Contains(chartUrl.Path, "@") + refAlreadyHasTagOrDigest := strings.Contains(chartURL.Path, ":") || strings.Contains(chartURL.Path, "@") if refAlreadyHasTagOrDigest { - return chartUrl, nil + return chartURL, nil } - tag, err := c.getOciTag(chartUrl.String(), version) + tag, err := c.getOciTag(chartURL.String(), version) if err != nil { return nil, err } - chartUrl.Path = fmt.Sprintf("%s:%s", chartUrl.Path, tag) + chartURL.Path = fmt.Sprintf("%s:%s", chartURL.Path, tag) - return chartUrl, nil + return chartURL, nil } func (c *ChartDownloader) getOciTag(ref, version string) (string, error) {