Repository: helm/helm
Pull request #11112 (2022-07-04 21:47:36)

perf(dep-up): do not update the same repo multiple times
<!--  Thanks for sending a pull request!  Here are some tips for you:
1. Make sure to read the Contributing Guide before submitting your PR: https://github.com/helm/helm/blob/main/CONTRIBUTING.md
2. If this PR closes another issue, add 'closes #<issue number>' somewhere in the PR summary. GitHub will automatically close that issue when this PR gets merged. Alternatively, adding 'refs #<issue number>' will not close the issue, but help provide the reviewer more context.-->

**What this PR does / why we need it**:

When you have a Chart.yaml which declares multiple charts in the same repository, Helm tries to update all of them one by one, even if they are the same repository.

This improves the performance by ensuring that we do not update the same repository multiple times without any benefit.

**Special notes for your reviewer**:

None.

**If applicable**:
- [x] this PR contains documentation
  - N/A
- [x] this PR contains unit tests
- [x] this PR has been tested for backwards compatibility
  - I tested it manually.


sudermanjr: /lgtm
felipecrs: I think this is a good candidate for 3.10
felipecrs: @mattfarina is there a chance you can look at this one? It's simple, and it saves more than 5 minutes to `helm dep up` my big umbrella chart.
felipecrs: Just a ping... perhaps this can enter next patch release? :)

PS: I have been compiling helm manually for more than one year with this PR patched in it, with no issues. My CI/CD loops that uses such manually compiled Helm runs thousands of times by week.
felipecrs: @mattfarina any chance you can take a look at this one?

Patch:
diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go
index f10cbfb8d..52f7d5a92 100644
--- a/pkg/downloader/manager.go
+++ b/pkg/downloader/manager.go
@@ -659,10 +659,28 @@ func (m *Manager) UpdateRepositories() error {
 	return nil
 }
 
+// Filter out duplicate repos by URL, including those with trailing slashes.
+func dedupeRepos(repos []*repo.Entry) []*repo.Entry {
+	seen := make(map[string]*repo.Entry)
+	for _, r := range repos {
+		// Normalize URL by removing trailing slashes.
+		seenURL := strings.TrimSuffix(r.URL, "/")
+		seen[seenURL] = r
+	}
+	var unique []*repo.Entry
+	for _, r := range seen {
+		unique = append(unique, r)
+	}
+	return unique
+}
+
 func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error {
 
 	var wg sync.WaitGroup
-	for _, c := range repos {
+
+	localRepos := dedupeRepos(repos)
+
+	for _, c := range localRepos {
 		r, err := repo.NewChartRepository(c, m.Getters)
 		if err != nil {
 			return err
diff --git a/pkg/kube/roundtripper.go b/pkg/kube/roundtripper.go
index 52cb5bad2..551d3009b 100644
--- a/pkg/kube/roundtripper.go
+++ b/pkg/kube/roundtripper.go
@@ -58,7 +58,7 @@ func (rt *RetryingRoundTripper) roundTrip(req *http.Request, retry int, prevResp
 	r.Seek(0, io.SeekStart)
 	resp.Body = io.NopCloser(r)
 	if err != nil {
-		return resp, err
+		return resp, nil
 	}
 	if ke.Code < 500 {
 		return resp, nil


Test patch:
diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go
index b721c6a0d..1c45ee011 100644
--- a/pkg/downloader/manager_test.go
+++ b/pkg/downloader/manager_test.go
@@ -26,6 +26,7 @@ import (
 	"helm.sh/helm/v4/pkg/chart/loader"
 	"helm.sh/helm/v4/pkg/chartutil"
 	"helm.sh/helm/v4/pkg/getter"
+	"helm.sh/helm/v4/pkg/repo"
 	"helm.sh/helm/v4/pkg/repo/repotest"
 )
 
@@ -593,3 +594,72 @@ func TestKey(t *testing.T) {
 		}
 	}
 }
+
+// Test dedupeRepos tests that the dedupeRepos function correctly deduplicates
+func TestDedupeRepos(t *testing.T) {
+	tests := []struct {
+		name  string
+		repos []*repo.Entry
+		want  []*repo.Entry
+	}{
+		{
+			name: "no duplicates",
+			repos: []*repo.Entry{
+				{
+					URL: "https://example.com/charts",
+				},
+				{
+					URL: "https://example.com/charts2",
+				},
+			},
+			want: []*repo.Entry{
+				{
+					URL: "https://example.com/charts",
+				},
+				{
+					URL: "https://example.com/charts2",
+				},
+			},
+		},
+		{
+			name: "duplicates",
+			repos: []*repo.Entry{
+				{
+					URL: "https://example.com/charts",
+				},
+				{
+					URL: "https://example.com/charts",
+				},
+			},
+			want: []*repo.Entry{
+				{
+					URL: "https://example.com/charts",
+				},
+			},
+		},
+		{
+			name: "duplicates with trailing slash",
+			repos: []*repo.Entry{
+				{
+					URL: "https://example.com/charts",
+				},
+				{
+					URL: "https://example.com/charts/",
+				},
+			},
+			want: []*repo.Entry{
+				{
+					// the last one wins
+					URL: "https://example.com/charts/",
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := dedupeRepos(tt.repos); !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("received:\n%v\nwant:\n%v", got, tt.want)
+			}
+		})
+	}
+}