chore: fix elseif and ifElseChain issues from gocritic (#32289)

* chore: fix elseif and ifElseChain issues from gocritic

Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>

* test: enhance assertions in OCIInstaller and list tests using testify

Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>

---------

Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>

.golangci.yml

@@ -87,10 +87,8 @@ linters:
         - badCall
         - commentedOutCode
         - deferInLoop
-        - elseif
         - exposedSyncMutex
         - hugeParam
-        - ifElseChain
         - importShadow
         - paramTypeCombine
         - ptrToRefParam

internal/chart/v3/lint/rules/chartfile.go

@@ -160,14 +160,14 @@ func validateChartVersion(cf *chart.Metadata) error {
 
 func validateChartMaintainer(cf *chart.Metadata) error {
 	for _, maintainer := range cf.Maintainers {
-		if maintainer == nil {
+		switch {
+		case maintainer == nil:
 			return errors.New("a maintainer entry is empty")
-		}
-		if maintainer.Name == "" {
+		case maintainer.Name == "":
 			return errors.New("each maintainer requires a name")
-		} else if maintainer.Email != "" && !govalidator.IsEmail(maintainer.Email) {
+		case maintainer.Email != "" && !govalidator.IsEmail(maintainer.Email):
 			return fmt.Errorf("invalid email '%s' for maintainer '%s'", maintainer.Email, maintainer.Name)
-		} else if maintainer.URL != "" && !govalidator.IsURL(maintainer.URL) {
+		case maintainer.URL != "" && !govalidator.IsURL(maintainer.URL):
 			return fmt.Errorf("invalid url '%s' for maintainer '%s'", maintainer.URL, maintainer.Name)
 		}
 	}

internal/plugin/installer/installer.go

@@ -136,15 +136,16 @@ func Update(i Installer) error {
 
 // NewForSource determines the correct Installer for the given source.
 func NewForSource(source, version string) (installer Installer, err error) {
-	if strings.HasPrefix(source, registry.OCIScheme+"://") {
+	switch {
+	case strings.HasPrefix(source, registry.OCIScheme+"://"):
 		// Source is an OCI registry reference
 		installer, err = NewOCIInstaller(source)
-	} else if isLocalReference(source) {
+	case isLocalReference(source):
 		// Source is a local directory
 		installer, err = NewLocalInstaller(source)
-	} else if isRemoteHTTPArchive(source) {
+	case isRemoteHTTPArchive(source):
 		installer, err = NewHTTPInstaller(source)
-	} else {
+	default:
 		installer, err = NewVCSInstaller(source, version)
 	}
 

internal/plugin/installer/oci_installer_test.go

@@ -33,6 +33,8 @@ import (
 
 	"github.com/opencontainers/go-digest"
 	ocispec "github.com/opencontainers/image-spec/specs-go/v1"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"helm.sh/helm/v4/internal/test/ensure"
 	"helm.sh/helm/v4/pkg/cli"
@@ -415,18 +417,11 @@ func TestOCIInstaller_Install_WithGetterOptions(t *testing.T) {
 			// Install the plugin
 			err = Install(installer)
 			if tc.wantErr {
-				if err == nil {
-					t.Error("Expected installation to fail, but it succeeded")
-				}
+				require.Error(t, err, "Expected installation to fail, but it succeeded")
 			} else {
-				if err != nil {
-					t.Errorf("Expected installation to succeed, got error: %v", err)
-				} else {
-					// Verify plugin was installed to the actual path
-					if !isPlugin(actualPath) {
-						t.Errorf("Expected plugin directory %s to contain plugin.yaml", actualPath)
-					}
-				}
+				require.NoError(t, err, "Expected installation to succeed, got error: %v", err)
+				// Verify plugin was installed to the actual path
+				assert.True(t, isPlugin(actualPath), "Expected plugin directory %s to contain plugin.yaml", actualPath)
 			}
 		})
 	}

pkg/chart/common/util/coalesce.go

@@ -336,20 +336,22 @@ func coalesceTablesFullKey(printf printFn, dst, src map[string]any, prefix strin
 	// values.
 	for key, val := range src {
 		fullkey := concatPrefix(prefix, key)
-		if dv, ok := dst[key]; ok && !merge && dv == nil && srcOriginalNonNil[key] {
+		dv, ok := dst[key]
+		switch {
+		case ok && !merge && dv == nil && srcOriginalNonNil[key]:
 			// When coalescing (not merging), if dst has nil and src has a non-nil
 			// value, the user is nullifying a chart default - remove the key.
 			// But if src also has nil (or key not in src), preserve the nil
 			delete(dst, key)
-		} else if !ok {
+		case !ok:
 			dst[key] = val
-		} else if istable(val) {
+		case istable(val):
 			if istable(dv) {
 				coalesceTablesFullKey(printf, dv.(map[string]any), val.(map[string]any), fullkey, merge)
 			} else {
 				printf("warning: cannot overwrite table with non table for %s (%v)", fullkey, val)
 			}
-		} else if istable(dv) && val != nil {
+		case istable(dv) && val != nil:
 			printf("warning: destination for %s is a table. Ignoring non-table value (%v)", fullkey, val)
 		}
 	}

pkg/chart/v2/lint/rules/chartfile.go

@@ -171,14 +171,14 @@ func validateChartVersionStrictSemVerV2(cf *chart.Metadata) error {
 
 func validateChartMaintainer(cf *chart.Metadata) error {
 	for _, maintainer := range cf.Maintainers {
-		if maintainer == nil {
+		switch {
+		case maintainer == nil:
 			return errors.New("a maintainer entry is empty")
-		}
-		if maintainer.Name == "" {
+		case maintainer.Name == "":
 			return errors.New("each maintainer requires a name")
-		} else if maintainer.Email != "" && !govalidator.IsEmail(maintainer.Email) {
+		case maintainer.Email != "" && !govalidator.IsEmail(maintainer.Email):
 			return fmt.Errorf("invalid email '%s' for maintainer '%s'", maintainer.Email, maintainer.Name)
-		} else if maintainer.URL != "" && !govalidator.IsURL(maintainer.URL) {
+		case maintainer.URL != "" && !govalidator.IsURL(maintainer.URL):
 			return fmt.Errorf("invalid url '%s' for maintainer '%s'", maintainer.URL, maintainer.Name)
 		}
 	}

pkg/cmd/list_test.go

@@ -20,6 +20,9 @@ import (
 	"testing"
 	"time"
 
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
 	chart "helm.sh/helm/v4/pkg/chart/v2"
 	"helm.sh/helm/v4/pkg/release/common"
 	release "helm.sh/helm/v4/pkg/release/v1"
@@ -348,13 +351,8 @@ func TestReleaseListWriter(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			writer := newReleaseListWriter(tt.releases, tt.timeFormat, tt.noHeaders, tt.noColor)
 
-			if writer == nil {
-				t.Error("Expected writer to be non-nil")
-			} else {
-				if len(writer.releases) != len(tt.releases) {
-					t.Errorf("Expected %d releases, got %d", len(tt.releases), len(writer.releases))
-				}
-			}
+			require.NotNil(t, writer, "Expected writer to be non-nil")
+			assert.Len(t, writer.releases, len(tt.releases), "Expected %d releases, got %d", len(tt.releases), len(writer.releases))
 		})
 	}
 }

pkg/cmd/registry_login.go

@@ -96,14 +96,15 @@ func getUsernamePassword(usernameOpt string, passwordOpt string, passwordFromStd
 	username := usernameOpt
 	password := passwordOpt
 
-	if passwordFromStdinOpt {
+	switch {
+	case passwordFromStdinOpt:
 		passwordFromStdin, err := io.ReadAll(os.Stdin)
 		if err != nil {
 			return "", "", err
 		}
 		password = strings.TrimSuffix(string(passwordFromStdin), "\n")
 		password = strings.TrimSuffix(password, "\r")
-	} else if password == "" {
+	case password == "":
 		if username == "" {
 			username, err = readLine("Username: ", false)
 			if err != nil {
@@ -126,7 +127,7 @@ func getUsernamePassword(usernameOpt string, passwordOpt string, passwordFromStd
 				return "", "", errors.New("password required")
 			}
 		}
-	} else {
+	default:
 		slog.Warn("using --password via the CLI is insecure. Use --password-stdin")
 	}
 

pkg/kube/client_test.go

@@ -1400,18 +1400,9 @@ func TestIsReachable(t *testing.T) {
 			err := client.IsReachable()
 
 			if tt.expectError {
-				if err == nil {
-					t.Error("expected error but got nil")
-					return
-				}
-
-				if !strings.Contains(err.Error(), tt.errorContains) {
-					t.Errorf("expected error message to contain '%s', got: %v", tt.errorContains, err)
-				}
+				require.ErrorContains(t, err, tt.errorContains)
 			} else {
-				if err != nil {
-					t.Errorf("expected no error but got: %v", err)
-				}
+				require.NoError(t, err)
 			}
 		})
 	}

pkg/provenance/sign_test.go

@@ -362,41 +362,28 @@ func TestClearSignError(t *testing.T) {
 
 func TestVerify(t *testing.T) {
 	signer, err := NewFromFiles(testKeyfile, testPubfile)
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 
 	// Read the chart file data
 	archiveData, err := os.ReadFile(testChartfile)
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 
 	// Read the signature file data
 	sigData, err := os.ReadFile(testSigBlock)
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 
-	if ver, err := signer.Verify(archiveData, sigData, filepath.Base(testChartfile)); err != nil {
-		t.Errorf("Failed to pass verify. Err: %s", err)
-	} else if ver.FileHash == "" {
-		t.Error("Verification is missing hash.")
-	} else if ver.SignedBy == nil {
-		t.Error("No SignedBy field")
-	} else if ver.FileName != filepath.Base(testChartfile) {
-		t.Errorf("FileName is unexpectedly %q", ver.FileName)
-	}
+	ver, err := signer.Verify(archiveData, sigData, filepath.Base(testChartfile))
+	require.NoError(t, err, "Failed to pass verify")
+	assert.NotEmpty(t, ver.FileHash, "Verification is missing hash.")
+	assert.NotNil(t, ver.SignedBy, "No SignedBy field")
+	assert.Equalf(t, filepath.Base(testChartfile), ver.FileName, "FileName is unexpectedly %q", ver.FileName)
 
 	// Read the tampered signature file data
 	tamperedSigData, err := os.ReadFile(testTamperedSigBlock)
-	if err != nil {
-		t.Fatal(err)
-	}
+	require.NoError(t, err)
 
-	if _, err = signer.Verify(archiveData, tamperedSigData, filepath.Base(testChartfile)); err == nil {
-		t.Errorf("Expected %s to fail.", testTamperedSigBlock)
-	}
+	_, err = signer.Verify(archiveData, tamperedSigData, filepath.Base(testChartfile))
+	require.Errorf(t, err, "Expected %s to fail.", testTamperedSigBlock)
 
 	switch err.(type) {
 	case pgperrors.SignatureError:

pkg/pusher/ocipusher_test.go

@@ -24,6 +24,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+
 	"helm.sh/helm/v4/pkg/registry"
 )
 
@@ -378,16 +380,12 @@ func TestOCIPusher_Push_ChartOperations(t *testing.T) {
 			err = pusher.Push(chartRef, tt.href)
 
 			if tt.expectError {
-				if err == nil {
-					t.Fatal("Expected error but got none")
-				}
-				if tt.errorContains != "" && !strings.Contains(err.Error(), tt.errorContains) {
-					t.Errorf("Expected error containing %q, got %q", tt.errorContains, err.Error())
+				require.Error(t, err)
+				if tt.errorContains != "" {
+					require.ErrorContains(t, err, tt.errorContains)
 				}
 			} else {
-				if err != nil {
-					t.Fatalf("Unexpected error: %v", err)
-				}
+				require.NoError(t, err)
 			}
 		})
 	}

pkg/registry/client.go

@@ -852,10 +852,8 @@ func (c *Client) ValidateReference(ref, version string, u *url.URL) (string, *ur
 	if version == "" {
 		// Use OCI URI tag as default
 		version = registryReference.Tag
-	} else {
-		if registryReference.Tag != "" && registryReference.Tag != version {
-			return "", nil, fmt.Errorf("chart reference and version mismatch: %s is not %s", version, registryReference.Tag)
-		}
+	} else if registryReference.Tag != "" && registryReference.Tag != version {
+		return "", nil, fmt.Errorf("chart reference and version mismatch: %s is not %s", version, registryReference.Tag)
 	}
 
 	if registryReference.Digest != "" {

pkg/registry/registry_test.go

@@ -200,7 +200,8 @@ func teardown(suite *TestRegistry) {
 
 func initCompromisedRegistryTestServer() string {
 	s := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if strings.Contains(r.URL.Path, "manifests") {
+		switch {
+		case strings.Contains(r.URL.Path, "manifests"):
 			w.Header().Set("Content-Type", "application/vnd.oci.image.manifest.v1+json")
 			w.WriteHeader(http.StatusOK)
 
@@ -217,17 +218,17 @@ func initCompromisedRegistryTestServer() string {
     }
   ]
 }`, ConfigMediaType, ChartLayerMediaType)
-		} else if r.URL.Path == "/v2/testrepo/supposedlysafechart/blobs/sha256:a705ee2789ab50a5ba20930f246dbd5cc01ff9712825bb98f57ee8414377f133" {
+		case r.URL.Path == "/v2/testrepo/supposedlysafechart/blobs/sha256:a705ee2789ab50a5ba20930f246dbd5cc01ff9712825bb98f57ee8414377f133":
 			w.Header().Set("Content-Type", "application/json")
 			w.WriteHeader(http.StatusOK)
 			_, _ = w.Write([]byte("{\"name\":\"mychart\",\"version\":\"0.1.0\",\"description\":\"A Helm chart for Kubernetes\\n" +
 				"an 'application' or a 'library' chart.\",\"apiVersion\":\"v2\",\"appVersion\":\"1.16.0\",\"type\":" +
 				"\"application\"}"))
-		} else if r.URL.Path == "/v2/testrepo/supposedlysafechart/blobs/sha256:ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb" {
+		case r.URL.Path == "/v2/testrepo/supposedlysafechart/blobs/sha256:ca978112ca1bbdcafac231b39a23dc4da786eff8147c4e72b9807785afee48bb":
 			w.Header().Set("Content-Type", ChartLayerMediaType)
 			w.WriteHeader(http.StatusOK)
 			_, _ = w.Write([]byte("b"))
-		} else {
+		default:
 			w.WriteHeader(http.StatusInternalServerError)
 		}
 	}))

pkg/registry/transport.go

@@ -71,11 +71,12 @@ func (t *LoggingTransport) RoundTrip(req *http.Request) (resp *http.Response, er
 
 	slog.Debug(req.Method, "id", id, "url", req.URL, "header", logHeader(req.Header))
 	resp, err = t.RoundTripper.RoundTrip(req)
-	if err != nil {
+	switch {
+	case err != nil:
 		slog.Debug("Response"[:len(req.Method)], "id", id, "error", err)
-	} else if resp != nil {
+	case resp != nil:
 		slog.Debug("Response"[:len(req.Method)], "id", id, "status", resp.Status, "header", logHeader(resp.Header), "body", logResponseBody(resp))
-	} else {
+	default:
 		slog.Debug("Response"[:len(req.Method)], "id", id, "response", "nil")
 	}
 

pkg/storage/driver/memory_test.go

@@ -261,27 +261,20 @@ func TestMemoryDelete(t *testing.T) {
 	}
 	startLen := len(start)
 	for _, tt := range tests {
-		ts.SetNamespace(tt.namespace)
+		t.Run(tt.desc, func(t *testing.T) {
+			ts.SetNamespace(tt.namespace)
 
-		rel, err := ts.Delete(tt.key)
-		var rls *rspb.Release
-		if err == nil {
-			rls = convertReleaserToV1(t, rel)
-		}
-		if err != nil {
-			if !tt.err {
-				t.Fatalf("Failed %q to get '%s': %q\n", tt.desc, tt.key, err)
+			rel, err := ts.Delete(tt.key)
+			if tt.err {
+				require.Errorf(t, err, "Did not get expected error for %q '%s'\n", tt.desc, tt.key)
+			} else {
+				require.NoErrorf(t, err, "Failed %q to get '%s'", tt.desc, tt.key)
+				rls := convertReleaserToV1(t, rel)
+				require.Equalf(t, tt.key, fmt.Sprintf("%s.v%d", rls.Name, rls.Version), "Asked for delete on %s, but deleted %d", tt.key, rls.Version)
 			}
-			continue
-		} else if tt.err {
-			t.Fatalf("Did not get expected error for %q '%s'\n", tt.desc, tt.key)
-		} else if fmt.Sprintf("%s.v%d", rls.Name, rls.Version) != tt.key {
-			t.Fatalf("Asked for delete on %s, but deleted %d", tt.key, rls.Version)
-		}
-		_, err = ts.Get(tt.key)
-		if err == nil {
-			t.Error("Expected an error when asking for a deleted key")
-		}
+			_, err = ts.Get(tt.key)
+			require.Error(t, err, "Expected an error when asking for a deleted key")
+		})
 	}
 
 	// Make sure that the deleted records are gone.