From 16787d65f5091cf5c5e4f2884ef0027ada117465 Mon Sep 17 00:00:00 2001 From: Matthieu MOREL Date: Fri, 10 Jul 2026 09:11:55 +0200 Subject: [PATCH] feat(linters): add new revive rules for better code quality Signed-off-by: Matthieu MOREL --- .golangci.yml | 10 +++++ internal/chart/v3/loader/load_test.go | 27 +++++-------- internal/chart/v3/util/save.go | 9 ++--- internal/plugin/installer/local_installer.go | 9 ++--- internal/plugin/runtime_subprocess.go | 6 +-- internal/third_party/dep/fs/fs.go | 31 +++++++-------- pkg/action/action.go | 7 ++-- pkg/action/upgrade.go | 7 ++-- pkg/chart/v2/loader/load_test.go | 19 ++++----- pkg/chart/v2/util/save.go | 9 ++--- pkg/cmd/install.go | 41 ++++++++++---------- pkg/cmd/upgrade.go | 39 +++++++++---------- pkg/downloader/manager.go | 23 +++++------ pkg/registry/client.go | 5 +-- pkg/storage/driver/sql.go | 6 +-- 15 files changed, 113 insertions(+), 135 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 1cd720d47..e839ea6a9 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -122,6 +122,16 @@ linters: enable-default-rules: true max-open-files: 2048 rules: + - name: early-return + arguments: + - "preserve-scope" + - name: if-return + - name: indent-error-flow + arguments: + - "preserve-scope" + - name: superfluous-else + arguments: + - "preserve-scope" - name: unnecessary-format - name: var-naming arguments: diff --git a/internal/chart/v3/loader/load_test.go b/internal/chart/v3/loader/load_test.go index a493104bf..1c5845da6 100644 --- a/internal/chart/v3/loader/load_test.go +++ b/internal/chart/v3/loader/load_test.go @@ -31,6 +31,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + chart "helm.sh/helm/v4/internal/chart/v3" "helm.sh/helm/v4/pkg/chart/common" "helm.sh/helm/v4/pkg/chart/loader/archive" @@ -100,41 +102,30 @@ func TestBomTestData(t *testing.T) { } archive, err := os.ReadFile("testdata/frobnitz_with_bom.tgz") - if err != nil { - t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err) - } + require.NoErrorf(t, err, "Error reading archive frobnitz_with_bom.tgz") unzipped, err := gzip.NewReader(bytes.NewReader(archive)) - if err != nil { - t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err) - } + require.NoErrorf(t, err, "Error reading archive frobnitz_with_bom.tgz") defer unzipped.Close() for _, testFile := range testFiles { data := make([]byte, 3) err := unzipped.Reset(bytes.NewReader(archive)) - if err != nil { - t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err) - } + require.NoErrorf(t, err, "Error reading archive frobnitz_with_bom.tgz") tr := tar.NewReader(unzipped) for { file, err := tr.Next() if errors.Is(err, io.EOF) { break } - if err != nil { - t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err) - } + require.NoErrorf(t, err, "Error reading archive frobnitz_with_bom.tgz") if file != nil && strings.EqualFold(file.Name, testFile) { _, err := tr.Read(data) - if err != nil { - t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err) - } else { + if err == nil { break } + t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err) } } - if !bytes.Equal(data, utf8bom) { - t.Fatalf("Test file has no BOM or is invalid: frobnitz_with_bom.tgz/%s", testFile) - } + require.Truef(t, bytes.Equal(data, utf8bom), "Test file has no BOM or is invalid: frobnitz_with_bom.tgz/%s", testFile) } } diff --git a/internal/chart/v3/util/save.go b/internal/chart/v3/util/save.go index 942e736ed..09235c1ea 100644 --- a/internal/chart/v3/util/save.go +++ b/internal/chart/v3/util/save.go @@ -114,13 +114,12 @@ func Save(c *chart.Chart, outDir string) (string, error) { filename = filepath.Join(outDir, filename) dir := filepath.Dir(filename) if stat, err := os.Stat(dir); err != nil { - if errors.Is(err, fs.ErrNotExist) { - if err2 := os.MkdirAll(dir, 0o755); err2 != nil { - return "", err2 - } - } else { + if !errors.Is(err, fs.ErrNotExist) { return "", fmt.Errorf("stat %s: %w", dir, err) } + if err2 := os.MkdirAll(dir, 0o755); err2 != nil { + return "", err2 + } } else if !stat.IsDir() { return "", fmt.Errorf("is not a directory: %s", dir) } diff --git a/internal/plugin/installer/local_installer.go b/internal/plugin/installer/local_installer.go index 8d067612f..a7a6caa14 100644 --- a/internal/plugin/installer/local_installer.go +++ b/internal/plugin/installer/local_installer.go @@ -204,14 +204,13 @@ func (i *LocalInstaller) GetVerificationData() (archiveData, provData []byte, fi provFile := i.Source + ".prov" i.provData, err = os.ReadFile(provFile) if err != nil { - if os.IsNotExist(err) { - // If provenance file doesn't exist, set provData to nil - // The verification logic will handle this gracefully - i.provData = nil - } else { + if !os.IsNotExist(err) { // If file exists but can't be read (permissions, etc), return error return nil, nil, "", fmt.Errorf("failed to access provenance file %s: %w", provFile, err) } + // If provenance file doesn't exist, set provData to nil + // The verification logic will handle this gracefully + i.provData = nil } } diff --git a/internal/plugin/runtime_subprocess.go b/internal/plugin/runtime_subprocess.go index 3c0af4a4e..b921587c1 100644 --- a/internal/plugin/runtime_subprocess.go +++ b/internal/plugin/runtime_subprocess.go @@ -125,11 +125,7 @@ func (r *SubprocessPluginRuntime) InvokeWithEnv(main string, argv []string, env cmd.Stdout = stdout cmd.Stderr = stderr - if err := executeCmd(cmd, r.metadata.Name); err != nil { - return err - } - - return nil + return executeCmd(cmd, r.metadata.Name) } func (r *SubprocessPluginRuntime) InvokeHook(event string) error { diff --git a/internal/third_party/dep/fs/fs.go b/internal/third_party/dep/fs/fs.go index 2607c73ac..4a8a736f7 100644 --- a/internal/third_party/dep/fs/fs.go +++ b/internal/third_party/dep/fs/fs.go @@ -156,24 +156,23 @@ func CopyFile(src, dst string) (err error) { if sym, err := IsSymlink(src); err != nil { return fmt.Errorf("symlink check failed: %w", err) } else if sym { - if err := cloneSymlink(src, dst); err != nil { - if runtime.GOOS == "windows" { - // If cloning the symlink fails on Windows because the user - // does not have the required privileges, ignore the error and - // fall back to copying the file contents. - // - // ERROR_PRIVILEGE_NOT_HELD is 1314 (0x522): - // https://msdn.microsoft.com/en-us/library/windows/desktop/ms681385(v=vs.85).aspx - lerr := &os.LinkError{} - if errors.As(err, &lerr) && !errors.Is(lerr.Err, syscall.Errno(1314)) { - return err - } - } else { - return err - } - } else { + err := cloneSymlink(src, dst) + if err == nil { return nil } + if runtime.GOOS != "windows" { + return err + } + // If cloning the symlink fails on Windows because the user + // does not have the required privileges, ignore the error and + // fall back to copying the file contents. + // + // ERROR_PRIVILEGE_NOT_HELD is 1314 (0x522): + // https://msdn.microsoft.com/en-us/library/windows/desktop/ms681385(v=vs.85).aspx + lerr := &os.LinkError{} + if errors.As(err, &lerr) && !errors.Is(lerr.Err, syscall.Errno(1314)) { + return err + } } in, err := os.Open(src) diff --git a/pkg/action/action.go b/pkg/action/action.go index 8105083f9..bcc91816c 100644 --- a/pkg/action/action.go +++ b/pkg/action/action.go @@ -551,12 +551,11 @@ func (cfg *Configuration) getCapabilities() (*common.Capabilities, error) { // See https://github.com/kubernetes/kubernetes/issues/72051#issuecomment-521157642 apiVersions, err := GetVersionSet(dc) if err != nil { - if discovery.IsGroupDiscoveryFailedError(err) { - cfg.Logger().Warn("the kubernetes server has an orphaned API service", slog.Any("error", err)) - cfg.Logger().Warn("to fix this, kubectl delete apiservice ") - } else { + if !discovery.IsGroupDiscoveryFailedError(err) { return nil, fmt.Errorf("could not get apiVersions from Kubernetes: %w", err) } + cfg.Logger().Warn("the kubernetes server has an orphaned API service", slog.Any("error", err)) + cfg.Logger().Warn("to fix this, kubectl delete apiservice ") } cfg.Capabilities = &common.Capabilities{ diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go index 98ac44690..7f66ceefb 100644 --- a/pkg/action/upgrade.go +++ b/pkg/action/upgrade.go @@ -261,12 +261,11 @@ func (u *Upgrade) prepareUpgrade(ctx context.Context, name string, chart *chartv return nil, nil, false, cerr } if err != nil { - if errors.Is(err, driver.ErrNoDeployedReleases) && - (lastRelease.Info.Status == rcommon.StatusFailed || lastRelease.Info.Status == rcommon.StatusSuperseded) { - currentRelease = lastRelease - } else { + if !errors.Is(err, driver.ErrNoDeployedReleases) || + (lastRelease.Info.Status != rcommon.StatusFailed && lastRelease.Info.Status != rcommon.StatusSuperseded) { return nil, nil, false, err } + currentRelease = lastRelease } } diff --git a/pkg/chart/v2/loader/load_test.go b/pkg/chart/v2/loader/load_test.go index e422c30de..46d17e899 100644 --- a/pkg/chart/v2/loader/load_test.go +++ b/pkg/chart/v2/loader/load_test.go @@ -31,6 +31,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + "helm.sh/helm/v4/pkg/chart/common" "helm.sh/helm/v4/pkg/chart/loader/archive" chart "helm.sh/helm/v4/pkg/chart/v2" @@ -111,30 +113,23 @@ func TestBomTestData(t *testing.T) { for _, testFile := range testFiles { data := make([]byte, 3) err := unzipped.Reset(bytes.NewReader(archive)) - if err != nil { - t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err) - } + require.NoErrorf(t, err, "Error reading archive frobnitz_with_bom.tgz") tr := tar.NewReader(unzipped) for { file, err := tr.Next() if errors.Is(err, io.EOF) { break } - if err != nil { - t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err) - } + require.NoErrorf(t, err, "Error reading archive frobnitz_with_bom.tgz") if file != nil && strings.EqualFold(file.Name, testFile) { _, err := tr.Read(data) - if err != nil { - t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err) - } else { + if err == nil { break } + t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err) } } - if !bytes.Equal(data, utf8bom) { - t.Fatalf("Test file has no BOM or is invalid: frobnitz_with_bom.tgz/%s", testFile) - } + require.Truef(t, bytes.Equal(data, utf8bom), "Test file has no BOM or is invalid: frobnitz_with_bom.tgz/%s", testFile) } } diff --git a/pkg/chart/v2/util/save.go b/pkg/chart/v2/util/save.go index d8189c8bf..5444ca218 100644 --- a/pkg/chart/v2/util/save.go +++ b/pkg/chart/v2/util/save.go @@ -114,13 +114,12 @@ func Save(c *chart.Chart, outDir string) (string, error) { filename = filepath.Join(outDir, filename) dir := filepath.Dir(filename) if stat, err := os.Stat(dir); err != nil { - if errors.Is(err, fs.ErrNotExist) { - if err2 := os.MkdirAll(dir, 0o755); err2 != nil { - return "", err2 - } - } else { + if !errors.Is(err, fs.ErrNotExist) { return "", fmt.Errorf("stat %s: %w", dir, err) } + if err2 := os.MkdirAll(dir, 0o755); err2 != nil { + return "", err2 + } } else if !stat.IsDir() { return "", fmt.Errorf("is not a directory: %s", dir) } diff --git a/pkg/cmd/install.go b/pkg/cmd/install.go index 832968ff0..c7aa2c542 100644 --- a/pkg/cmd/install.go +++ b/pkg/cmd/install.go @@ -301,29 +301,28 @@ func runInstall(args []string, client *action.Install, valueOpts *values.Options // As of Helm 2.4.0, this is treated as a stopping condition: // https://github.com/helm/helm/issues/2209 if err := action.CheckDependencies(chartRequested, req); err != nil { - if client.DependencyUpdate { - man := &downloader.Manager{ - Out: out, - ChartPath: cp, - Keyring: client.Keyring, - SkipUpdate: false, - Getters: p, - RepositoryConfig: settings.RepositoryConfig, - RepositoryCache: settings.RepositoryCache, - ContentCache: settings.ContentCache, - Debug: settings.Debug, - RegistryClient: client.GetRegistryClient(), - } - if err := man.Update(); err != nil { - return nil, err - } - // Reload the chart with the updated Chart.lock file. - if chartRequested, err = loader.Load(cp); err != nil { - return nil, fmt.Errorf("failed reloading chart after repo update: %w", err) - } - } else { + if !client.DependencyUpdate { return nil, fmt.Errorf("an error occurred while checking for chart dependencies. You may need to run 'helm dependency build' to fetch missing dependencies: %w", err) } + man := &downloader.Manager{ + Out: out, + ChartPath: cp, + Keyring: client.Keyring, + SkipUpdate: false, + Getters: p, + RepositoryConfig: settings.RepositoryConfig, + RepositoryCache: settings.RepositoryCache, + ContentCache: settings.ContentCache, + Debug: settings.Debug, + RegistryClient: client.GetRegistryClient(), + } + if err := man.Update(); err != nil { + return nil, err + } + // Reload the chart with the updated Chart.lock file. + if chartRequested, err = loader.Load(cp); err != nil { + return nil, fmt.Errorf("failed reloading chart after repo update: %w", err) + } } } diff --git a/pkg/cmd/upgrade.go b/pkg/cmd/upgrade.go index 9ec4f4ef2..a6aa96129 100644 --- a/pkg/cmd/upgrade.go +++ b/pkg/cmd/upgrade.go @@ -206,28 +206,27 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command { if req := ac.MetaDependencies(); len(req) > 0 { if err := action.CheckDependencies(ch, req); err != nil { err = fmt.Errorf("an error occurred while checking for chart dependencies. You may need to run 'helm dependency build' to fetch missing dependencies: %w", err) - if client.DependencyUpdate { - man := &downloader.Manager{ - Out: out, - ChartPath: chartPath, - Keyring: client.Keyring, - SkipUpdate: false, - Getters: p, - RepositoryConfig: settings.RepositoryConfig, - RepositoryCache: settings.RepositoryCache, - ContentCache: settings.ContentCache, - Debug: settings.Debug, - } - if err := man.Update(); err != nil { - return err - } - // Reload the chart with the updated Chart.lock file. - if ch, err = loader.Load(chartPath); err != nil { - return fmt.Errorf("failed reloading chart after repo update: %w", err) - } - } else { + if !client.DependencyUpdate { return err } + man := &downloader.Manager{ + Out: out, + ChartPath: chartPath, + Keyring: client.Keyring, + SkipUpdate: false, + Getters: p, + RepositoryConfig: settings.RepositoryConfig, + RepositoryCache: settings.RepositoryCache, + ContentCache: settings.ContentCache, + Debug: settings.Debug, + } + if err := man.Update(); err != nil { + return err + } + // Reload the chart with the updated Chart.lock file. + if ch, err = loader.Load(chartPath); err != nil { + return fmt.Errorf("failed reloading chart after repo update: %w", err) + } } } diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 683f96007..e5c015f24 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -118,16 +118,15 @@ func (m *Manager) Build() error { } if sum, err := resolver.HashReq(req, lock.Dependencies); err != nil || sum != lock.Digest { + if c.Metadata.APIVersion != chart.APIVersionV1 { + return errors.New("the lock file (Chart.lock) is out of sync with the dependencies file (Chart.yaml). Please update the dependencies with 'helm dependency update'") + } // If lock digest differs and chart is apiVersion v1, it maybe because the lock was built // with Helm 2 and therefore should be checked with Helm v2 hash // Fix for: https://github.com/helm/helm/issues/7233 - if c.Metadata.APIVersion == chart.APIVersionV1 { - log.Println("warning: a valid Helm v3 hash was not found. Checking against Helm v2 hash...") - if v2Sum != lock.Digest { - return errors.New("the lock file (requirements.lock) is out of sync with the dependencies file (requirements.yaml). Please update the dependencies") - } - } else { - return errors.New("the lock file (Chart.lock) is out of sync with the dependencies file (Chart.yaml). Please update the dependencies with 'helm dependency update'") + log.Println("warning: a valid Helm v3 hash was not found. Checking against Helm v2 hash...") + if v2Sum != lock.Digest { + return errors.New("the lock file (requirements.lock) is out of sync with the dependencies file (requirements.yaml). Please update the dependencies") } } @@ -365,16 +364,12 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { } // TODO: this should probably be refactored to be a []error, so we can capture and provide more information rather than "last error wins". - if saveError == nil { - // now we can move all downloaded charts to destPath and delete outdated dependencies - if err := m.safeMoveDeps(deps, tmpPath, destPath); err != nil { - return err - } - } else { + if saveError != nil { fmt.Fprintln(m.Out, "Save error occurred: ", saveError) return saveError } - return nil + // now we can move all downloaded charts to destPath and delete outdated dependencies + return m.safeMoveDeps(deps, tmpPath, destPath) } func parseOCIRef(chartRef string) (string, string, error) { diff --git a/pkg/registry/client.go b/pkg/registry/client.go index a14afdd1f..7a1c8f449 100644 --- a/pkg/registry/client.go +++ b/pkg/registry/client.go @@ -484,12 +484,11 @@ func (c *Client) processChartPull(genericResult *GenericPullResult, operation *p var provMissing bool if operation.withProv && provDescriptor == nil { - if operation.ignoreMissingProv { - provMissing = true - } else { + if !operation.ignoreMissingProv { return nil, fmt.Errorf("manifest does not contain a layer with mediatype %s", ProvLayerMediaType) } + provMissing = true } // Build chart-specific result diff --git a/pkg/storage/driver/sql.go b/pkg/storage/driver/sql.go index f3dd053a2..88d24123d 100644 --- a/pkg/storage/driver/sql.go +++ b/pkg/storage/driver/sql.go @@ -404,12 +404,12 @@ func (s *SQL) Query(labels map[string]string) ([]release.Releaser, error) { } sort.Strings(keys) for _, key := range keys { - if _, ok := labelMap[key]; ok { - sb = sb.Where(sq.Eq{key: labels[key]}) - } else { + _, ok := labelMap[key] + if !ok { s.Logger().Debug("unknown label", "key", key) return nil, fmt.Errorf("unknown label %s", key) } + sb = sb.Where(sq.Eq{key: labels[key]}) } // If a namespace was specified, we only list releases from that namespace