Merge pull request #32321 from mmorel-35/gocritic

refactor: enable several checks from gocritic
2026-08-10 17:14:52 +00:00 · 2026-07-14 13:38:07 -07:00
parent c3b336af15 9662fdd73b
commit 66c1232dc7
32 changed files with 55 additions and 62 deletions
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -84,19 +84,14 @@ linters:
    gocritic:
      disabled-checks:
        - appendAssign
-        - appendCombine
-        - assignOp
        - badCall
        - commentedOutCode
-        - commentFormatting
        - deferInLoop
        - elseif
        - exposedSyncMutex
-        - filepathJoin
        - hugeParam
        - ifElseChain
        - importShadow
-        - nilValReturn
        - paramTypeCombine
        - ptrToRefParam
        - rangeValCopy
--- a/internal/chart/v3/lint/rules/values_test.go
+++ b/internal/chart/v3/lint/rules/values_test.go
@@ -26,7 +26,7 @@ import (
 	"helm.sh/helm/v4/internal/test/ensure"
 )

-var nonExistingValuesFilePath = filepath.Join("/fake/dir", "values.yaml")
+var nonExistingValuesFilePath = filepath.FromSlash("/fake/dir/values.yaml")

 const testSchema = `
 {
--- a/internal/chart/v3/util/create_test.go
+++ b/internal/chart/v3/util/create_test.go
@@ -121,7 +121,7 @@ func TestCreate_Overwrite(t *testing.T) {

 	dir := filepath.Join(tdir, "foo")

-	tplname := filepath.Join(dir, "templates/hpa.yaml")
+	tplname := filepath.Join(dir, "templates", "hpa.yaml")
 	writeFile(tplname, []byte("FOO"))

 	// Now re-run the create
--- a/internal/release/v2/util/manifest.go
+++ b/internal/release/v2/util/manifest.go
@@ -64,7 +64,7 @@ func SplitManifests(bigFile string) map[string]string {

 		d = strings.TrimLeftFunc(d, unicode.IsSpace)
 		res[fmt.Sprintf(tpl, count)] = d
-		count = count + 1
+		count++
 	}
 	return res
 }
--- a/internal/third_party/dep/fs/fs_test.go
+++ b/internal/third_party/dep/fs/fs_test.go
@@ -358,9 +358,9 @@ func TestCopyFileSymlink(t *testing.T) {
 	tempdir := t.TempDir()

 	testcases := map[string]string{
-		filepath.Join("./testdata/symlinks/file-symlink"):         filepath.Join(tempdir, "dst-file"),
-		filepath.Join("./testdata/symlinks/windows-file-symlink"): filepath.Join(tempdir, "windows-dst-file"),
-		filepath.Join("./testdata/symlinks/invalid-symlink"):      filepath.Join(tempdir, "invalid-symlink"),
+		filepath.Join(".", "testdata", "symlinks", "file-symlink"):         filepath.Join(tempdir, "dst-file"),
+		filepath.Join(".", "testdata", "symlinks", "windows-file-symlink"): filepath.Join(tempdir, "windows-dst-file"),
+		filepath.Join(".", "testdata", "symlinks", "invalid-symlink"):      filepath.Join(tempdir, "invalid-symlink"),
 	}

 	for symlink, dst := range testcases {
--- a/pkg/action/dependency.go
+++ b/pkg/action/dependency.go
@@ -201,7 +201,7 @@ func (d *Dependency) printDependencies(chartpath string, out io.Writer, c *chart
 // printMissing prints warnings about charts that are present on disk, but are
 // not in Chart.yaml.
 func (d *Dependency) printMissing(chartpath string, out io.Writer, reqs []*chart.Dependency) {
-	folder := filepath.Join(chartpath, "charts/*")
+	folder := filepath.Join(chartpath, "charts", "*")
 	files, err := filepath.Glob(folder)
 	if err != nil {
 		fmt.Fprintln(out, err)
--- a/pkg/action/install_test.go
+++ b/pkg/action/install_test.go
@@ -868,21 +868,21 @@ func TestInstallReleaseOutputDir(t *testing.T) {
 		t.Fatalf("Failed install: %s", err)
 	}

-	_, err = os.Stat(filepath.Join(dir, "hello/templates/goodbye"))
+	_, err = os.Stat(filepath.Join(dir, "hello", "templates", "goodbye"))
 	req.NoError(err)

-	_, err = os.Stat(filepath.Join(dir, "hello/templates/hello"))
+	_, err = os.Stat(filepath.Join(dir, "hello", "templates", "hello"))
 	req.NoError(err)

-	_, err = os.Stat(filepath.Join(dir, "hello/templates/with-partials"))
+	_, err = os.Stat(filepath.Join(dir, "hello", "templates", "with-partials"))
 	req.NoError(err)

-	_, err = os.Stat(filepath.Join(dir, "hello/templates/rbac"))
+	_, err = os.Stat(filepath.Join(dir, "hello", "templates", "rbac"))
 	req.NoError(err)

-	test.AssertGoldenFile(t, filepath.Join(dir, "hello/templates/rbac"), "rbac.txt")
+	test.AssertGoldenFile(t, filepath.Join(dir, "hello", "templates", "rbac"), "rbac.txt")

-	_, err = os.Stat(filepath.Join(dir, "hello/templates/empty"))
+	_, err = os.Stat(filepath.Join(dir, "hello", "templates", "empty"))
 	is.ErrorIs(err, fs.ErrNotExist)
 }

@@ -905,21 +905,21 @@ func TestInstallOutputDirWithReleaseName(t *testing.T) {
 		t.Fatalf("Failed install: %s", err)
 	}

-	_, err = os.Stat(filepath.Join(newDir, "hello/templates/goodbye"))
+	_, err = os.Stat(filepath.Join(newDir, "hello", "templates", "goodbye"))
 	req.NoError(err)

-	_, err = os.Stat(filepath.Join(newDir, "hello/templates/hello"))
+	_, err = os.Stat(filepath.Join(newDir, "hello", "templates", "hello"))
 	req.NoError(err)

-	_, err = os.Stat(filepath.Join(newDir, "hello/templates/with-partials"))
+	_, err = os.Stat(filepath.Join(newDir, "hello", "templates", "with-partials"))
 	req.NoError(err)

-	_, err = os.Stat(filepath.Join(newDir, "hello/templates/rbac"))
+	_, err = os.Stat(filepath.Join(newDir, "hello", "templates", "rbac"))
 	req.NoError(err)

-	test.AssertGoldenFile(t, filepath.Join(newDir, "hello/templates/rbac"), "rbac.txt")
+	test.AssertGoldenFile(t, filepath.Join(newDir, "hello", "templates", "rbac"), "rbac.txt")

-	_, err = os.Stat(filepath.Join(newDir, "hello/templates/empty"))
+	_, err = os.Stat(filepath.Join(newDir, "hello", "templates", "empty"))
 	is.ErrorIs(err, fs.ErrNotExist)
 }

--- a/pkg/action/list.go
+++ b/pkg/action/list.go
@@ -178,7 +178,7 @@ func (l *List) Run() ([]ri.Releaser, error) {
 	}

 	if results == nil {
-		return results, nil
+		return nil, nil
 	}

 	rresults, err := releaseListToV1List(results)
--- a/pkg/chart/v2/lint/rules/values_test.go
+++ b/pkg/chart/v2/lint/rules/values_test.go
@@ -26,7 +26,7 @@ import (
 	"helm.sh/helm/v4/internal/test/ensure"
 )

-var nonExistingValuesFilePath = filepath.Join("/fake/dir", "values.yaml")
+var nonExistingValuesFilePath = filepath.FromSlash("/fake/dir/values.yaml")

 const testSchema = `
 {
--- a/pkg/chart/v2/util/create_test.go
+++ b/pkg/chart/v2/util/create_test.go
@@ -121,7 +121,7 @@ func TestCreate_Overwrite(t *testing.T) {

 	dir := filepath.Join(tdir, "foo")

-	tplname := filepath.Join(dir, "templates/hpa.yaml")
+	tplname := filepath.Join(dir, "templates", "hpa.yaml")
 	writeFile(tplname, []byte("FOO"))

 	// Now re-run the create
--- a/pkg/cmd/dependency_build_test.go
+++ b/pkg/cmd/dependency_build_test.go
@@ -72,7 +72,7 @@ func TestDependencyBuildCmd(t *testing.T) {
 	}

 	// Make sure the actual file got downloaded.
-	expect := filepath.Join(rootDir, chartname, "charts/reqtest-0.1.0.tgz")
+	expect := filepath.Join(rootDir, chartname, "charts", "reqtest-0.1.0.tgz")
 	if _, err := os.Stat(expect); err != nil {
 		t.Fatal(err)
 	}
--- a/pkg/cmd/repo_add_test.go
+++ b/pkg/cmd/repo_add_test.go
@@ -49,7 +49,7 @@ func TestRepoAddCmd(t *testing.T) {
 	)
 	defer srv2.Stop()

-	tmpdir := filepath.Join(t.TempDir(), "path-component.yaml/data")
+	tmpdir := filepath.Join(t.TempDir(), "path-component.yaml", "data")
 	if err := os.MkdirAll(tmpdir, 0o777); err != nil {
 		t.Fatal(err)
 	}
--- a/pkg/cmd/repo_remove_test.go
+++ b/pkg/cmd/repo_remove_test.go
@@ -171,7 +171,7 @@ func TestRepoRemoveCompletion(t *testing.T) {

 	rootDir := t.TempDir()
 	repoFile := filepath.Join(rootDir, "repositories.yaml")
-	repoCache := filepath.Join(rootDir, "cache/")
+	repoCache := filepath.Join(rootDir, "cache")

 	var testRepoNames = []string{"foo", "bar", "baz"}

--- a/pkg/cmd/search_repo.go
+++ b/pkg/cmd/search_repo.go
@@ -382,15 +382,15 @@ func compListCharts(toComplete string, includeFiles bool) ([]string, cobra.Shell

 	directive := cobra.ShellCompDirectiveDefault
 	if noFile {
-		directive = directive | cobra.ShellCompDirectiveNoFileComp
+		directive |= cobra.ShellCompDirectiveNoFileComp
 	}
 	if noSpace {
-		directive = directive | cobra.ShellCompDirectiveNoSpace
+		directive |= cobra.ShellCompDirectiveNoSpace
 	}
 	if !includeFiles {
 		// If we should not include files in the completions,
 		// we should disable file completion
-		directive = directive | cobra.ShellCompDirectiveNoFileComp
+		directive |= cobra.ShellCompDirectiveNoFileComp
 	}
 	return completions, directive
 }
--- a/pkg/cmd/show_test.go
+++ b/pkg/cmd/show_test.go
@@ -76,7 +76,6 @@ func TestShowPreReleaseChart(t *testing.T) {
 				outdir,
 				contentTmp,
 			)
-			//_, out, err := executeActionCommand(cmd)
 			_, _, err := executeActionCommand(cmd)
 			if err != nil {
 				if tt.fail {
--- a/pkg/cmd/template_test.go
+++ b/pkg/cmd/template_test.go
@@ -40,7 +40,7 @@ func TestTemplateCmd(t *testing.T) {
 		},
 		{
 			name:   "check values files",
-			cmd:    fmt.Sprintf("template '%s' --values '%s'", chartPath, filepath.Join(chartPath, "/charts/subchartA/values.yaml")),
+			cmd:    fmt.Sprintf("template '%s' --values '%s'", chartPath, filepath.Join(chartPath, "charts", "subchartA", "values.yaml")),
 			golden: "output/template-values-files.txt",
 		},
 		{
--- a/pkg/downloader/cache_test.go
+++ b/pkg/downloader/cache_test.go
@@ -117,6 +117,6 @@ func TestDiskCache_fileName(t *testing.T) {
 	cache := &DiskCache{Root: "/tmp/cache"}
 	key := sha256.Sum256([]byte("some data"))

-	assert.Equal(t, filepath.Join("/tmp/cache", "13", "1307990e6ba5ca145eb35e99182a9bec46531bc54ddf656a602c780fa0240dee.chart"), cache.fileName(key, CacheChart))
-	assert.Equal(t, filepath.Join("/tmp/cache", "13", "1307990e6ba5ca145eb35e99182a9bec46531bc54ddf656a602c780fa0240dee.prov"), cache.fileName(key, CacheProv))
+	assert.Equal(t, filepath.FromSlash("/tmp/cache/13/1307990e6ba5ca145eb35e99182a9bec46531bc54ddf656a602c780fa0240dee.chart"), cache.fileName(key, CacheChart))
+	assert.Equal(t, filepath.FromSlash("/tmp/cache/13/1307990e6ba5ca145eb35e99182a9bec46531bc54ddf656a602c780fa0240dee.prov"), cache.fileName(key, CacheProv))
 }
--- a/pkg/downloader/chart_downloader_test.go
+++ b/pkg/downloader/chart_downloader_test.go
@@ -278,7 +278,7 @@ func TestDownloadTo_TLS(t *testing.T) {
 			getter.WithTLSClientConfig(
 				"",
 				"",
-				filepath.Join("../../testdata/rootca.crt"),
+				filepath.FromSlash("../../testdata/rootca.crt"),
 			),
 		},
 	}
--- a/pkg/downloader/manager.go
+++ b/pkg/downloader/manager.go
@@ -764,7 +764,7 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*
 	}
 	url, err = repo.FindChartInRepoURL(repoURL, name, m.Getters, repo.WithChartVersion(version), repo.WithClientTLS(certFile, keyFile, caFile))
 	if err == nil {
-		return url, username, password, false, false, "", "", "", err
+		return url, username, password, false, false, "", "", "", nil
 	}
 	err = fmt.Errorf("chart %s not found in %s: %w", name, repoURL, err)
 	return url, username, password, false, false, "", "", "", err
--- a/pkg/engine/funcs_test.go
+++ b/pkg/engine/funcs_test.go
@@ -28,7 +28,7 @@ import (
 )

 func TestFuncs(t *testing.T) {
-	//TODO write tests for failure cases
+	// TODO write tests for failure cases
 	tests := []struct {
 		tpl, expect string
 		vars        any
--- a/pkg/getter/httpgetter_test.go
+++ b/pkg/getter/httpgetter_test.go
@@ -225,7 +225,7 @@ func TestDownload(t *testing.T) {
 	// A different host is provided for the WithURL from the one used for Get
 	u2, _ := url.ParseRequestURI(crossAuthSrv.URL)
 	host := strings.Split(u2.Host, ":")
-	host[0] = host[0] + "a"
+	host[0] += "a"
 	u2.Host = strings.Join(host, ":")
 	httpgetter, err = NewHTTPGetter(
 		WithURL(u2.String()),
@@ -260,7 +260,7 @@ func TestDownload(t *testing.T) {
 	// A different host is provided for the WithURL from the one used for Get
 	u2, _ = url.ParseRequestURI(crossAuthSrv.URL)
 	host = strings.Split(u2.Host, ":")
-	host[0] = host[0] + "a"
+	host[0] += "a"
 	u2.Host = strings.Join(host, ":")
 	httpgetter, err = NewHTTPGetter(
 		WithURL(u2.String()),
--- a/pkg/helmpath/lazypath_unix_test.go
+++ b/pkg/helmpath/lazypath_unix_test.go
@@ -38,7 +38,7 @@ func TestDataPath(t *testing.T) {

 	t.Setenv(xdg.DataHomeEnvVar, "/tmp")

-	expected = filepath.Join("/tmp", appName, testFile)
+	expected = filepath.FromSlash("/tmp/" + appName + "/" + testFile)

 	assert.Equal(t, expected, lazy.dataPath(testFile))
 }
@@ -50,7 +50,7 @@ func TestConfigPath(t *testing.T) {

 	t.Setenv(xdg.ConfigHomeEnvVar, "/tmp")

-	expected = filepath.Join("/tmp", appName, testFile)
+	expected = filepath.FromSlash("/tmp/" + appName + "/" + testFile)

 	assert.Equal(t, expected, lazy.configPath(testFile))
 }
@@ -62,7 +62,7 @@ func TestCachePath(t *testing.T) {

 	t.Setenv(xdg.CacheHomeEnvVar, "/tmp")

-	expected = filepath.Join("/tmp", appName, testFile)
+	expected = filepath.FromSlash("/tmp/" + appName + "/" + testFile)

 	assert.Equal(t, expected, lazy.cachePath(testFile))
 }
--- a/pkg/kube/ready_test.go
+++ b/pkg/kube/ready_test.go
@@ -213,7 +213,7 @@ func Test_ReadyChecker_IsReady_Deployment(t *testing.T) {
 			wantErr:    true,
 		},
 		{
-			name: "IsReady Deployments", //TODO fix this one
+			name: "IsReady Deployments", // TODO fix this one
 			fields: fields{
 				client:        fake.NewClientset(),
 				checkJobs:     true,
--- a/pkg/registry/client_scope_test.go
+++ b/pkg/registry/client_scope_test.go
@@ -76,7 +76,7 @@ func (suite *RegistryScopeTestSuite) Test_1_Check_Push_Request_Scope() {
 	_, err = suite.RegistryClient.Push(chartData, ref, PushOptCreationTime(testingChartCreationTime))
 	suite.Require().Error(err, "error pushing good ref because auth server doesn't give proper token")

-	//check the url that authentication server received
+	// check the url that authentication server received
 	select {
 	case urlStr := <-requestURL:
 		u, err := url.Parse(urlStr)
@@ -122,7 +122,7 @@ func (suite *RegistryScopeTestSuite) Test_2_Check_Pull_Request_Scope() {
 	_, err = suite.RegistryClient.Pull(ref)
 	suite.Require().Error(err, "error pulling a simple chart because auth server doesn't give proper token")

-	//check the url that authentication server received
+	// check the url that authentication server received
 	select {
 	case urlStr := <-requestURL:
 		u, err := url.Parse(urlStr)
--- a/pkg/registry/registry_test.go
+++ b/pkg/registry/registry_test.go
@@ -150,7 +150,7 @@ func setup(suite *TestRegistry, tlsEnabled, insecure bool, auth string) {
 		suite.Require().NoError(err, "no error finding free port for test auth server")
 		defer ln.Close()

-		//set test auth server host
+		// set test auth server host
 		suite.AuthServerHost = ln.Addr().String()

 		config.Auth = configuration.Auth{
--- a/pkg/release/v1/util/manifest.go
+++ b/pkg/release/v1/util/manifest.go
@@ -64,7 +64,7 @@ func SplitManifests(bigFile string) map[string]string {

 		d = strings.TrimLeftFunc(d, unicode.IsSpace)
 		res[fmt.Sprintf(tpl, count)] = d
-		count = count + 1
+		count++
 	}
 	return res
 }
--- a/pkg/repo/v1/index.go
+++ b/pkg/repo/v1/index.go
@@ -318,7 +318,7 @@ func IndexDirectory(dir, baseURL string) (*IndexFile, error) {
 	if err != nil {
 		return nil, err
 	}
-	moreArchives, err := filepath.Glob(filepath.Join(dir, "**/*.tgz"))
+	moreArchives, err := filepath.Glob(filepath.Join(dir, "**", "*.tgz"))
 	if err != nil {
 		return nil, err
 	}
--- a/pkg/storage/driver/sql_test.go
+++ b/pkg/storage/driver/sql_test.go
@@ -112,12 +112,14 @@ func TestSQLGet(t *testing.T) {

 func TestSQLList(t *testing.T) {
 	releases := []*rspb.Release{}
-	releases = append(releases, releaseStub("key-1", 1, "default", common.StatusUninstalled))
-	releases = append(releases, releaseStub("key-2", 1, "default", common.StatusUninstalled))
-	releases = append(releases, releaseStub("key-3", 1, "default", common.StatusDeployed))
-	releases = append(releases, releaseStub("key-4", 1, "default", common.StatusDeployed))
-	releases = append(releases, releaseStub("key-5", 1, "default", common.StatusSuperseded))
-	releases = append(releases, releaseStub("key-6", 1, "default", common.StatusSuperseded))
+	releases = append(releases,
+		releaseStub("key-1", 1, "default", common.StatusUninstalled),
+		releaseStub("key-2", 1, "default", common.StatusUninstalled),
+		releaseStub("key-3", 1, "default", common.StatusDeployed),
+		releaseStub("key-4", 1, "default", common.StatusDeployed),
+		releaseStub("key-5", 1, "default", common.StatusSuperseded),
+		releaseStub("key-6", 1, "default", common.StatusSuperseded),
+	)

 	sqlDriver, mock := newTestFixtureSQL(t)

--- a/pkg/storage/storage_test.go
+++ b/pkg/storage/storage_test.go
@@ -324,7 +324,6 @@ func (d *MaxHistoryMockDriver) Name() string {
 }

 func TestMaxHistoryErrorHandling(t *testing.T) {
-	//func TestStorageRemoveLeastRecentWithError(t *testing.T) {
 	storage := Init(NewMaxHistoryMockDriver(driver.NewMemory()))

 	storage.MaxHistory = 1
--- a/pkg/strvals/literal_parser_test.go
+++ b/pkg/strvals/literal_parser_test.go
@@ -422,7 +422,7 @@ func TestParseLiteralNestedLevels(t *testing.T) {
 	for i := 1; i <= MaxNestedNameLevel+2; i++ {
 		tmpStr := fmt.Sprintf("name%d", i)
 		if i <= MaxNestedNameLevel+1 {
-			tmpStr = tmpStr + "."
+			tmpStr += "."
 		}
 		keyMultipleNestedLevels.WriteString(tmpStr)
 	}
--- a/pkg/strvals/parser.go
+++ b/pkg/strvals/parser.go
@@ -190,8 +190,6 @@ func (t *parser) key(data map[string]any, nestedNameLevel int) (reterr error) {
 				return err
 			}
 			return fmt.Errorf("key %q has no value", string(k))
-			//set(data, string(k), "")
-			//return err
 		case last == '[':
 			// We are in a list index context, so we need to set an index.
 			i, err := t.keyIndex()
--- a/pkg/strvals/parser_test.go
+++ b/pkg/strvals/parser_test.go
@@ -750,7 +750,7 @@ func TestParseSetNestedLevels(t *testing.T) {
 	for i := 1; i <= MaxNestedNameLevel+2; i++ {
 		tmpStr := fmt.Sprintf("name%d", i)
 		if i <= MaxNestedNameLevel+1 {
-			tmpStr = tmpStr + "."
+			tmpStr += "."
 		}
 		keyMultipleNestedLevels.WriteString(tmpStr)
 	}