Repository: helm/helm
Pull request #10024 (2021-08-13 12:18:31)

feat(repo update): Add flag for failing command if any update of a repo fails
**What this PR does / why we need it**:

The current implementation of `helm repo update` command is to return success (exit code 0) even if it fails to update 1, many or all repositories in the list.  The behaviour cannot be changed in Helm 3 as it would break backward compatibility (rightly or wrongly , users expect it to work in a certain way).  This PR adds a flag `fail-on-repo-update-fail` which when set will fail when any repo fails to update.

The addition of the flag for Helm 3, provides users with being able to test if any update of repositories fail without having to parse the output of the Helm command. A proposal on how the behaviour should work in future Helm major release will be submitted for Helm 4.  Check out #10016 for more details.

Partial #10016 

**Special notes for your reviewer**:

**If applicable**:
- [x] this PR contains documentation
- [x] this PR contains unit tests
- [x] this PR has been tested for backwards compatibility



Patch:
diff --git a/cmd/helm/repo_update.go b/cmd/helm/repo_update.go
index 43036847d..ae09f9195 100644
--- a/cmd/helm/repo_update.go
+++ b/cmd/helm/repo_update.go
@@ -41,10 +41,11 @@ To update all the repositories, use 'helm repo update'.
 var errNoRepositories = errors.New("no repositories found. You must add one before updating")
 
 type repoUpdateOptions struct {
-	update    func([]*repo.ChartRepository, io.Writer)
-	repoFile  string
-	repoCache string
-	names     []string
+	update               func([]*repo.ChartRepository, io.Writer, bool) error
+	repoFile             string
+	repoCache            string
+	names                []string
+	failOnRepoUpdateFail bool
 }
 
 func newRepoUpdateCmd(out io.Writer) *cobra.Command {
@@ -66,6 +67,13 @@ func newRepoUpdateCmd(out io.Writer) *cobra.Command {
 			return o.run(out)
 		},
 	}
+
+	f := cmd.Flags()
+
+	// Adding this flag for Helm 3 as stop gap functionality for https://github.com/helm/helm/issues/10016.
+	// This should be deprecated in Helm 4 by update to the behaviour of `helm repo update` command.
+	f.BoolVar(&o.failOnRepoUpdateFail, "fail-on-repo-update-fail", false, "update fails if any of the repository updates fail")
+
 	return cmd
 }
 
@@ -103,26 +111,34 @@ func (o *repoUpdateOptions) run(out io.Writer) error {
 		}
 	}
 
-	o.update(repos, out)
-	return nil
+	return o.update(repos, out, o.failOnRepoUpdateFail)
 }
 
-func updateCharts(repos []*repo.ChartRepository, out io.Writer) {
+func updateCharts(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool) error {
 	fmt.Fprintln(out, "Hang tight while we grab the latest from your chart repositories...")
 	var wg sync.WaitGroup
+	var repoFailList []string
 	for _, re := range repos {
 		wg.Add(1)
 		go func(re *repo.ChartRepository) {
 			defer wg.Done()
 			if _, err := re.DownloadIndexFile(); err != nil {
 				fmt.Fprintf(out, "...Unable to get an update from the %q chart repository (%s):\n\t%s\n", re.Config.Name, re.Config.URL, err)
+				repoFailList = append(repoFailList, re.Config.URL)
 			} else {
 				fmt.Fprintf(out, "...Successfully got an update from the %q chart repository\n", re.Config.Name)
 			}
 		}(re)
 	}
 	wg.Wait()
+
+	if len(repoFailList) > 0 && failOnRepoUpdateFail {
+		return errors.New(fmt.Sprintf("Failed to update the following repositories: %s",
+			repoFailList))
+	}
+
 	fmt.Fprintln(out, "Update Complete. ⎈Happy Helming!⎈")
+	return nil
 }
 
 func checkRequestedRepos(requestedRepos []string, validRepos []*repo.Entry) error {


Test patch:
diff --git a/cmd/helm/repo_update_test.go b/cmd/helm/repo_update_test.go
index d769c8aa7..cf2136ff0 100644
--- a/cmd/helm/repo_update_test.go
+++ b/cmd/helm/repo_update_test.go
@@ -35,10 +35,11 @@ func TestUpdateCmd(t *testing.T) {
 	var out bytes.Buffer
 	// Instead of using the HTTP updater, we provide our own for this test.
 	// The TestUpdateCharts test verifies the HTTP behavior independently.
-	updater := func(repos []*repo.ChartRepository, out io.Writer) {
+	updater := func(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool) error {
 		for _, re := range repos {
 			fmt.Fprintln(out, re.Config.Name)
 		}
+		return nil
 	}
 	o := &repoUpdateOptions{
 		update:   updater,
@@ -59,10 +60,11 @@ func TestUpdateCmdMultiple(t *testing.T) {
 	var out bytes.Buffer
 	// Instead of using the HTTP updater, we provide our own for this test.
 	// The TestUpdateCharts test verifies the HTTP behavior independently.
-	updater := func(repos []*repo.ChartRepository, out io.Writer) {
+	updater := func(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool) error {
 		for _, re := range repos {
 			fmt.Fprintln(out, re.Config.Name)
 		}
+		return nil
 	}
 	o := &repoUpdateOptions{
 		update:   updater,
@@ -84,10 +86,11 @@ func TestUpdateCmdInvalid(t *testing.T) {
 	var out bytes.Buffer
 	// Instead of using the HTTP updater, we provide our own for this test.
 	// The TestUpdateCharts test verifies the HTTP behavior independently.
-	updater := func(repos []*repo.ChartRepository, out io.Writer) {
+	updater := func(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool) error {
 		for _, re := range repos {
 			fmt.Fprintln(out, re.Config.Name)
 		}
+		return nil
 	}
 	o := &repoUpdateOptions{
 		update:   updater,
@@ -144,7 +147,7 @@ func TestUpdateCharts(t *testing.T) {
 	}
 
 	b := bytes.NewBuffer(nil)
-	updateCharts([]*repo.ChartRepository{r}, b)
+	updateCharts([]*repo.ChartRepository{r}, b, false)
 
 	got := b.String()
 	if strings.Contains(got, "Unable to get an update") {
@@ -159,3 +162,79 @@ func TestRepoUpdateFileCompletion(t *testing.T) {
 	checkFileCompletion(t, "repo update", false)
 	checkFileCompletion(t, "repo update repo1", false)
 }
+
+func TestUpdateChartsFail(t *testing.T) {
+	defer resetEnv()()
+	defer ensure.HelmHome(t)()
+
+	ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer ts.Stop()
+
+	var invalidURL = ts.URL() + "55"
+	r, err := repo.NewChartRepository(&repo.Entry{
+		Name: "charts",
+		URL:  invalidURL,
+	}, getter.All(settings))
+	if err != nil {
+		t.Error(err)
+	}
+
+	b := bytes.NewBuffer(nil)
+	if err := updateCharts([]*repo.ChartRepository{r}, b, false); err != nil {
+		t.Error("Repo update should not return error if update of repository fails")
+	}
+
+	got := b.String()
+	if !strings.Contains(got, "Unable to get an update") {
+		t.Errorf("Repo should have failed update but instead got: %q", got)
+	}
+	if !strings.Contains(got, "Update Complete.") {
+		t.Error("Update was not successful")
+	}
+}
+
+func TestUpdateChartsFailWithError(t *testing.T) {
+	defer resetEnv()()
+	defer ensure.HelmHome(t)()
+
+	ts, err := repotest.NewTempServerWithCleanup(t, "testdata/testserver/*.*")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer ts.Stop()
+
+	var invalidURL = ts.URL() + "55"
+	r, err := repo.NewChartRepository(&repo.Entry{
+		Name: "charts",
+		URL:  invalidURL,
+	}, getter.All(settings))
+	if err != nil {
+		t.Error(err)
+	}
+
+	b := bytes.NewBuffer(nil)
+	err = updateCharts([]*repo.ChartRepository{r}, b, true)
+	if err == nil {
+		t.Error("Repo update should return error because update of repository fails and 'fail-on-repo-update-fail' flag set")
+		return
+	}
+	var expectedErr = "Failed to update the following repositories"
+	var receivedErr = err.Error()
+	if !strings.Contains(receivedErr, expectedErr) {
+		t.Errorf("Expected error (%s) but got (%s) instead", expectedErr, receivedErr)
+	}
+	if !strings.Contains(receivedErr, invalidURL) {
+		t.Errorf("Expected invalid URL (%s) in error message but got (%s) instead", invalidURL, receivedErr)
+	}
+
+	got := b.String()
+	if !strings.Contains(got, "Unable to get an update") {
+		t.Errorf("Repo should have failed update but instead got: %q", got)
+	}
+	if strings.Contains(got, "Update Complete.") {
+		t.Error("Update was not successful and should return error message because 'fail-on-repo-update-fail' flag set")
+	}
+}