feat(linters): add new revive rules for better code quality

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

.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:

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 {
+	require.NoErrorf(t, err, "Error reading archive frobnitz_with_bom.tgz")
-		t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err)
-	}
 	unzipped, err := gzip.NewReader(bytes.NewReader(archive))
-	if err != nil {
+	require.NoErrorf(t, err, "Error reading archive frobnitz_with_bom.tgz")
-		t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err)
-	}
 	defer unzipped.Close()
 	for _, testFile := range testFiles {
 		data := make([]byte, 3)
 		err := unzipped.Reset(bytes.NewReader(archive))
-		if err != nil {
+		require.NoErrorf(t, err, "Error reading archive frobnitz_with_bom.tgz")
-			t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err)
-		}
 		tr := tar.NewReader(unzipped)
 		for {
 			file, err := tr.Next()
 			if errors.Is(err, io.EOF) {
 				break
 			}
-			if err != nil {
+			require.NoErrorf(t, err, "Error reading archive frobnitz_with_bom.tgz")
-				t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err)
-			}
 			if file != nil && strings.EqualFold(file.Name, testFile) {
 				_, err := tr.Read(data)
-				if err != nil {
+				if err == nil {
-					t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err)
-				} else {
 					break
 				}
+				t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err)
 			}
 		}
-		if !bytes.Equal(data, utf8bom) {
+		require.Truef(t, bytes.Equal(data, utf8bom), "Test file has no BOM or is invalid: frobnitz_with_bom.tgz/%s", testFile)
-			t.Fatalf("Test file has no BOM or is invalid: frobnitz_with_bom.tgz/%s", testFile)
-		}
 	}
 }
 

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 !errors.Is(err, fs.ErrNotExist) {
-			if err2 := os.MkdirAll(dir, 0o755); err2 != nil {
-				return "", err2
-			}
-		} else {
 			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)
 	}

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 !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 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
 		}
 	}
 

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 executeCmd(cmd, r.metadata.Name)
-		return err
-	}
-
-	return nil
 }
 
 func (r *SubprocessPluginRuntime) InvokeHook(event string) error {

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 {
+		err := cloneSymlink(src, dst)
-			if runtime.GOOS == "windows" {
+		if err == nil {
-				// 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 {
 			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)

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) {
+		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 <service-name>")
-		} else {
 			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 <service-name>")
 	}
 
 	cfg.Capabilities = &common.Capabilities{

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) &&
+			if !errors.Is(err, driver.ErrNoDeployedReleases) ||
-				(lastRelease.Info.Status == rcommon.StatusFailed || lastRelease.Info.Status == rcommon.StatusSuperseded) {
+				(lastRelease.Info.Status != rcommon.StatusFailed && lastRelease.Info.Status != rcommon.StatusSuperseded) {
-				currentRelease = lastRelease
-			} else {
 				return nil, nil, false, err
 			}
+			currentRelease = lastRelease
 		}
 	}
 

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 {
+		require.NoErrorf(t, err, "Error reading archive frobnitz_with_bom.tgz")
-			t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err)
-		}
 		tr := tar.NewReader(unzipped)
 		for {
 			file, err := tr.Next()
 			if errors.Is(err, io.EOF) {
 				break
 			}
-			if err != nil {
+			require.NoErrorf(t, err, "Error reading archive frobnitz_with_bom.tgz")
-				t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err)
-			}
 			if file != nil && strings.EqualFold(file.Name, testFile) {
 				_, err := tr.Read(data)
-				if err != nil {
+				if err == nil {
-					t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err)
-				} else {
 					break
 				}
+				t.Fatalf("Error reading archive frobnitz_with_bom.tgz: %s", err)
 			}
 		}
-		if !bytes.Equal(data, utf8bom) {
+		require.Truef(t, bytes.Equal(data, utf8bom), "Test file has no BOM or is invalid: frobnitz_with_bom.tgz/%s", testFile)
-			t.Fatalf("Test file has no BOM or is invalid: frobnitz_with_bom.tgz/%s", testFile)
-		}
 	}
 }
 

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 !errors.Is(err, fs.ErrNotExist) {
-			if err2 := os.MkdirAll(dir, 0o755); err2 != nil {
-				return "", err2
-			}
-		} else {
 			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)
 	}

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 {
+			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 {
 				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)
+			}
 		}
 	}
 

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 {
+					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 {
 						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)
+					}
 				}
 			}
 

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...")
-			log.Println("warning: a valid Helm v3 hash was not found. Checking against Helm v2 hash...")
+		if v2Sum != lock.Digest {
-			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")
-				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'")
 		}
 	}
 
@@ -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 {
+	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 {
 		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) {

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 {
+		if !operation.ignoreMissingProv {
-			provMissing = true
-		} else {
 			return nil, fmt.Errorf("manifest does not contain a layer with mediatype %s",
 				ProvLayerMediaType)
 		}
+		provMissing = true
 	}
 
 	// Build chart-specific result

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 {
+		_, ok := labelMap[key]
-			sb = sb.Where(sq.Eq{key: labels[key]})
+		if !ok {
-		} else {
 			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