From 26515f3bdca2c08c3233ae3a05bdc33aef0ba762 Mon Sep 17 00:00:00 2001 From: Matthieu MOREL Date: Fri, 3 Jul 2026 14:28:37 +0000 Subject: [PATCH 1/2] chore: fix elseif and ifElseChain issues from gocritic Signed-off-by: Matthieu MOREL --- .golangci.yml | 2 -- internal/chart/v3/lint/rules/chartfile.go | 10 +++--- internal/plugin/installer/installer.go | 9 ++--- .../plugin/installer/oci_installer_test.go | 6 ++-- pkg/chart/common/util/coalesce.go | 10 +++--- pkg/chart/v2/lint/rules/chartfile.go | 10 +++--- pkg/cmd/list_test.go | 6 ++-- pkg/cmd/registry_login.go | 7 ++-- pkg/kube/client_test.go | 13 ++----- pkg/provenance/sign_test.go | 35 ++++++------------- pkg/pusher/ocipusher_test.go | 14 ++++---- pkg/registry/client.go | 6 ++-- pkg/registry/registry_test.go | 9 ++--- pkg/registry/transport.go | 7 ++-- pkg/storage/driver/memory_test.go | 31 +++++++--------- 15 files changed, 71 insertions(+), 104 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 1cd720d47..76e61b845 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -90,11 +90,9 @@ linters: - commentedOutCode - commentFormatting - deferInLoop - - elseif - exposedSyncMutex - filepathJoin - hugeParam - - ifElseChain - importShadow - nilValReturn - paramTypeCombine diff --git a/internal/chart/v3/lint/rules/chartfile.go b/internal/chart/v3/lint/rules/chartfile.go index 6f51a1de5..db7ac61dc 100644 --- a/internal/chart/v3/lint/rules/chartfile.go +++ b/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) } } diff --git a/internal/plugin/installer/installer.go b/internal/plugin/installer/installer.go index 865570773..4728879f5 100644 --- a/internal/plugin/installer/installer.go +++ b/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) } diff --git a/internal/plugin/installer/oci_installer_test.go b/internal/plugin/installer/oci_installer_test.go index 650e46add..885366028 100644 --- a/internal/plugin/installer/oci_installer_test.go +++ b/internal/plugin/installer/oci_installer_test.go @@ -421,11 +421,9 @@ func TestOCIInstaller_Install_WithGetterOptions(t *testing.T) { } 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) - } + } else if !isPlugin(actualPath) { + t.Errorf("Expected plugin directory %s to contain plugin.yaml", actualPath) } } }) diff --git a/pkg/chart/common/util/coalesce.go b/pkg/chart/common/util/coalesce.go index 999eeb208..c1f73b16d 100644 --- a/pkg/chart/common/util/coalesce.go +++ b/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) } } diff --git a/pkg/chart/v2/lint/rules/chartfile.go b/pkg/chart/v2/lint/rules/chartfile.go index cb58bbac0..08508f46e 100644 --- a/pkg/chart/v2/lint/rules/chartfile.go +++ b/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) } } diff --git a/pkg/cmd/list_test.go b/pkg/cmd/list_test.go index 35153465a..615b6bc88 100644 --- a/pkg/cmd/list_test.go +++ b/pkg/cmd/list_test.go @@ -350,10 +350,8 @@ func TestReleaseListWriter(t *testing.T) { 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)) - } + } else if len(writer.releases) != len(tt.releases) { + t.Errorf("Expected %d releases, got %d", len(tt.releases), len(writer.releases)) } }) } diff --git a/pkg/cmd/registry_login.go b/pkg/cmd/registry_login.go index 1350fb244..bffc56445 100644 --- a/pkg/cmd/registry_login.go +++ b/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") } diff --git a/pkg/kube/client_test.go b/pkg/kube/client_test.go index 4a28fc5ed..b5f1f5f12 100644 --- a/pkg/kube/client_test.go +++ b/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) } }) } diff --git a/pkg/provenance/sign_test.go b/pkg/provenance/sign_test.go index faf402b43..3f8e654f9 100644 --- a/pkg/provenance/sign_test.go +++ b/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: diff --git a/pkg/pusher/ocipusher_test.go b/pkg/pusher/ocipusher_test.go index 52536dfd2..a4542500a 100644 --- a/pkg/pusher/ocipusher_test.go +++ b/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) } }) } diff --git a/pkg/registry/client.go b/pkg/registry/client.go index a14afdd1f..f300dccc8 100644 --- a/pkg/registry/client.go +++ b/pkg/registry/client.go @@ -853,10 +853,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 != "" { diff --git a/pkg/registry/registry_test.go b/pkg/registry/registry_test.go index afcf88db5..291e2d5eb 100644 --- a/pkg/registry/registry_test.go +++ b/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) } })) diff --git a/pkg/registry/transport.go b/pkg/registry/transport.go index 68e4ac587..beec43cdd 100644 --- a/pkg/registry/transport.go +++ b/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") } diff --git a/pkg/storage/driver/memory_test.go b/pkg/storage/driver/memory_test.go index f821e30d4..6040daf59 100644 --- a/pkg/storage/driver/memory_test.go +++ b/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. From 19cc25dc0044046cd7dc7a4cf83e55e191768d79 Mon Sep 17 00:00:00 2001 From: Matthieu MOREL Date: Tue, 7 Jul 2026 19:10:45 +0200 Subject: [PATCH 2/2] test: enhance assertions in OCIInstaller and list tests using testify Signed-off-by: Matthieu MOREL --- internal/plugin/installer/oci_installer_test.go | 15 ++++++--------- pkg/cmd/list_test.go | 10 +++++----- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/internal/plugin/installer/oci_installer_test.go b/internal/plugin/installer/oci_installer_test.go index 885366028..c3bebdb52 100644 --- a/internal/plugin/installer/oci_installer_test.go +++ b/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,16 +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) - // Verify plugin was installed to the actual path - } else 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) } }) } diff --git a/pkg/cmd/list_test.go b/pkg/cmd/list_test.go index 615b6bc88..f45616250 100644 --- a/pkg/cmd/list_test.go +++ b/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,11 +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)) }) } }