Repository: helm/helm (go)

Issue:
helm repo update returns 0 error code even when it fails
<!-- If you need help or think you have found a bug, please help us with your issue by entering the following information (otherwise you can delete this text): -->

Output of `helm version`: version.BuildInfo{Version:"v3.6.0", GitCommit:"7f2df6467771a75f5646b7f12afb408590ed1755", GitTreeState:"dirty", GoVersion:"go1.16.4"}

Output of `kubectl version`: Client Version: version.Info{Major:"1", Minor:"22", GitVersion:"v1.22.0", GitCommit:"c2b5237ccd9c0f1d600d3072634ca66cefdf272f", GitTreeState:"clean", BuildDate:"2021-08-04T17:56:19Z", GoVersion:"go1.16.6", Compiler:"gc", Platform:"darwin/amd64"}

Cloud Provider/Platform (AKS, GKE, Minikube etc.): AWS

### Unexpected behaviour and repro:

1. Add a helm repo with valid credentials.
2. Invalidate credentials on repo side.
3. Attempt to do a help repo update.

Result (at least for me):

Hang tight while we grab the latest from your chart repositories...
...Unable to get an update from the "starburstdata" chart repository (https://harbor.starburstdata.net/chartrepo/starburstdata):
	failed to fetch https://harbor.starburstdata.net/chartrepo/starburstdata/index.yaml : 401 Unauthorized
Update Complete. ⎈Happy Helming!⎈

Obviously the credentials are now invalid, so the update fails. However, the return code from this is 0:

~/git/bigbang % echo $?
0

### Expected behaviour

Non-zero error code

Pull request:
Error when failed repo update.
In Helm v3 we did not change exit codes for existing commands to maintain compat. A flag was introduced so a failure would result in a non-0 exit code. A note was left to make this the default in Helm v4. That's what this change does.

Closes #10016

<!--  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**:

**Special notes for your reviewer**:

**If applicable**:
- [ ] this PR contains user facing changes (the `docs needed` label should be applied if so)
- [ ] this PR contains unit tests
- [ ] this PR has been tested for backwards compatibility


Patch:
diff --git a/pkg/cmd/repo_update.go b/pkg/cmd/repo_update.go
index 25071377b..6590d9872 100644
--- a/pkg/cmd/repo_update.go
+++ b/pkg/cmd/repo_update.go
@@ -42,11 +42,10 @@ 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, bool) error
-	repoFile             string
-	repoCache            string
-	names                []string
-	failOnRepoUpdateFail bool
+	update    func([]*repo.ChartRepository, io.Writer) error
+	repoFile  string
+	repoCache string
+	names     []string
 }
 
 func newRepoUpdateCmd(out io.Writer) *cobra.Command {
@@ -69,12 +68,6 @@ func newRepoUpdateCmd(out io.Writer) *cobra.Command {
 		},
 	}
 
-	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
 }
 
@@ -112,10 +105,10 @@ func (o *repoUpdateOptions) run(out io.Writer) error {
 		}
 	}
 
-	return o.update(repos, out, o.failOnRepoUpdateFail)
+	return o.update(repos, out)
 }
 
-func updateCharts(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdateFail bool) error {
+func updateCharts(repos []*repo.ChartRepository, out io.Writer) error {
 	fmt.Fprintln(out, "Hang tight while we grab the latest from your chart repositories...")
 	var wg sync.WaitGroup
 	var repoFailList []string
@@ -133,7 +126,7 @@ func updateCharts(repos []*repo.ChartRepository, out io.Writer, failOnRepoUpdate
 	}
 	wg.Wait()
 
-	if len(repoFailList) > 0 && failOnRepoUpdateFail {
+	if len(repoFailList) > 0 {
 		return fmt.Errorf("Failed to update the following repositories: %s",
 			repoFailList)
 	}


Test patch:
diff --git a/pkg/cmd/repo_update_test.go b/pkg/cmd/repo_update_test.go
index 5b27a6dfb..6fc4c8f4b 100644
--- a/pkg/cmd/repo_update_test.go
+++ b/pkg/cmd/repo_update_test.go
@@ -34,7 +34,7 @@ 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, _ bool) error {
+	updater := func(repos []*repo.ChartRepository, out io.Writer) error {
 		for _, re := range repos {
 			fmt.Fprintln(out, re.Config.Name)
 		}
@@ -59,7 +59,7 @@ 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, _ bool) error {
+	updater := func(repos []*repo.ChartRepository, out io.Writer) error {
 		for _, re := range repos {
 			fmt.Fprintln(out, re.Config.Name)
 		}
@@ -85,7 +85,7 @@ 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, _ bool) error {
+	updater := func(repos []*repo.ChartRepository, out io.Writer) error {
 		for _, re := range repos {
 			fmt.Fprintln(out, re.Config.Name)
 		}
@@ -145,7 +145,7 @@ func TestUpdateCharts(t *testing.T) {
 	}
 
 	b := bytes.NewBuffer(nil)
-	updateCharts([]*repo.ChartRepository{r}, b, false)
+	updateCharts([]*repo.ChartRepository{r}, b)
 
 	got := b.String()
 	if strings.Contains(got, "Unable to get an update") {
@@ -161,39 +161,6 @@ func TestRepoUpdateFileCompletion(t *testing.T) {
 	checkFileCompletion(t, "repo update repo1", false)
 }
 
-func TestUpdateChartsFail(t *testing.T) {
-	defer resetEnv()()
-	ensure.HelmHome(t)
-
-	ts := repotest.NewTempServer(
-		t,
-		repotest.WithChartSourceGlob("testdata/testserver/*.*"),
-	)
-	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()()
 	ensure.HelmHome(t)
@@ -214,7 +181,7 @@ func TestUpdateChartsFailWithError(t *testing.T) {
 	}
 
 	b := bytes.NewBuffer(nil)
-	err = updateCharts([]*repo.ChartRepository{r}, b, true)
+	err = updateCharts([]*repo.ChartRepository{r}, b)
 	if err == nil {
 		t.Error("Repo update should return error because update of repository fails and 'fail-on-repo-update-fail' flag set")
 		return