upgrade: golangci-lint to v2 (#22648)

* fix: golangci-lint issues

* migrate golangci-lint config to v2

* upgrade: golangci-lint to v2.4.0

Author: sreeram77

.github/workflows/reusable-lint.yml

@@ -54,7 +54,7 @@ jobs:
       - name: Set golangci-lint version
         run: echo "GOLANGCI_LINT_VERSION=$(make --no-print-directory print-GOLANGCI_LINT_VERSION)" >> $GITHUB_ENV
       - name: lint-${{ matrix.directory }}
-        uses: golangci/golangci-lint-action@82d40c283aeb1f2b6595839195e95c2d6a49081b # v5.0.0
+        uses: golangci/golangci-lint-action@4afd733a84b1f43292c63897423277bb7f4313a9 # v8.0.0
         with:
           working-directory: ${{ matrix.directory }}
           version: ${{ env.GOLANGCI_LINT_VERSION }}

.golangci.yml

@@ -1,124 +1,118 @@
 # Copyright (c) HashiCorp, Inc.
 # SPDX-License-Identifier: BUSL-1.1
 
+version: "2"
+run:
+  concurrency: 4
 linters:
-  disable-all: true
+  default: none
   enable:
-    - gofmt
+    - depguard
+    - forbidigo
+    - gomodguard
     - govet
-    - unconvert
-    - staticcheck
     - ineffassign
+    - staticcheck
+    - unconvert
     - unparam
-    - forbidigo
-    - gomodguard
-    - depguard
-
-issues:
-  # Disable the default exclude list so that all excludes are explicitly
-  # defined in this file.
-  exclude-use-default: false
-
-  exclude-rules:
-    # Temp Ignore SA9004: only the first constant in this group has an explicit type
-    # https://staticcheck.io/docs/checks#SA9004
-    - linters: [staticcheck]
-      text: "SA9004:"
-
-    - linters: [staticcheck]
-      text: "SA1006:"
-
-    - linters: [staticcheck]
-      text: 'SA1019: "io/ioutil" has been deprecated since Go 1.16'
-
-    # Allow usage of deprecated values.
-    - linters: [ staticcheck ]
-      text: 'SA1019:'
-      path: "(agent/grpc-external|agent/grpc-internal)"
-
-    # An argument that always receives the same value is often not a problem.
-    - linters: [unparam]
-      text: "always receives"
-
-    - linters: [ unparam ]
-      text: 'result \d+ \(bool\) is always false'
-
-    # Often functions will implement an interface that returns an error without
-    # needing to return an error. Sometimes the error return value is unnecessary
-    # but a linter can not tell the difference.
-    - linters: [unparam]
-      text: 'result \d+ \(error\) is always nil'
-
-    # Allow unused parameters to start with an underscore. Arguments with a name
-    # of '_' are already ignored.
-    # Ignoring longer names that start with underscore allow for better
-    # self-documentation than a single underscore by itself.  Underscore arguments
-    # should generally only be used when a function is implementing an interface.
-    - linters: [unparam]
-      text: "`_[^`]*` is unused"
-
-    # Temp ignore some common unused parameters so that unparam can be added
-    # incrementally.
-    - linters: [unparam]
-      text: "`(t|resp|req|entMeta)` is unused"
-
-    # Temp ignore everything in _ce(_test).go and _ent(_test).go. Many of these
-    # could use underscore to ignore the unused arguments, but the "always returns"
-    # issue will likely remain in CE, and will need to be excluded.
-    - linters: [unparam]
-      path: "(_ce.go|_ce_test.go|_ent.go|_ent_test.go)"
-
-    - linters: [ staticcheck ]
-      text: 'SA1019:'
-
-linters-settings:
-  govet:
-    enable-all: true
-    disable:
-      - fieldalignment
-      - nilness
-      - shadow
-      - unusedwrite
-      - printf
-  gofmt:
-    simplify: true
-  forbidigo:
-    # Forbid the following identifiers (list of regexp).
-    # Format includes custom message based on https://github.com/ashanbrown/forbidigo/pull/11
-    forbid:
-      - '\bhtml\/template\b(# Use text/template instead)?'
-      - '\bioutil\b(# Use io and os packages instead of ioutil)?'
-      - '\brequire\.New\b(# Use package-level functions with explicit TestingT)?'
-      - '\bassert\.New\b(# Use package-level functions with explicit TestingT)?'
-    # Exclude godoc examples from forbidigo checks.
-    # Default: true
-    exclude_godoc_examples: false
-  gomodguard:
-    blocked:
-      # List of blocked modules.
-      modules:
-        # Blocked module.
-        - github.com/hashicorp/net-rpc-msgpackrpc:
-            recommendations:
-              - github.com/hashicorp/consul-net-rpc/net-rpc-msgpackrpc
-        - github.com/hashicorp/go-msgpack:
-            recommendations:
-              - github.com/hashicorp/consul-net-rpc/go-msgpack
-        - github.com/golang/protobuf:
-            recommendations:
-              - google.golang.org/protobuf
-
-  depguard:
+  settings:
+    depguard:
+      rules:
+        main:
+          deny:
+            - pkg: net/rpc
+              desc: only use forked copy in github.com/hashicorp/consul-net-rpc/net/rpc
+            - pkg: github.com/golang/protobuf
+              desc: only use google.golang.org/protobuf
+    forbidigo:
+      forbid:
+        - pattern: \bhtml\/template\b(# Use text/template instead)?
+        - pattern: \bioutil\b(# Use io and os packages instead of ioutil)?
+        - pattern: \brequire\.New\b(# Use package-level functions with explicit TestingT)?
+        - pattern: \bassert\.New\b(# Use package-level functions with explicit TestingT)?
+    gomodguard:
+      blocked:
+        modules:
+          - github.com/hashicorp/net-rpc-msgpackrpc:
+              recommendations:
+                - github.com/hashicorp/consul-net-rpc/net-rpc-msgpackrpc
+          - github.com/hashicorp/go-msgpack:
+              recommendations:
+                - github.com/hashicorp/consul-net-rpc/go-msgpack
+          - github.com/golang/protobuf:
+              recommendations:
+                - google.golang.org/protobuf
+    govet:
+      disable:
+        - fieldalignment
+        - nilness
+        - shadow
+        - unusedwrite
+        - printf
+      enable-all: true
+  exclusions:
+    generated: lax
     rules:
-      main:
-        listMode: lax
-        deny:
-        - pkg: net/rpc
-          desc: "only use forked copy in github.com/hashicorp/consul-net-rpc/net/rpc"
-        - pkg: github.com/golang/protobuf
-          desc: "only use google.golang.org/protobuf"
-
-run:
-  timeout: 10m
-  concurrency: 4
-  skip-dirs-use-default: false
+      - linters:
+          - staticcheck
+        text: 'SA9004:'
+      - linters:
+          - staticcheck
+        text: 'SA1006:'
+      - linters:
+          - staticcheck
+        text: 'SA1019: "io/ioutil" has been deprecated since Go 1.16'
+      - linters:
+          - staticcheck
+        path: (agent/grpc-external|agent/grpc-internal)
+        text: 'SA1019:'
+      - linters:
+          - unparam
+        text: always receives
+      - linters:
+          - unparam
+        text: result \d+ \(bool\) is always false
+      - linters:
+          - unparam
+        text: result \d+ \(error\) is always nil
+      - linters:
+          - unparam
+        text: '`_[^`]*` is unused'
+      - linters:
+          - unparam
+        text: '`(t|resp|req|entMeta)` is unused'
+      - linters:
+          - unparam
+        path: (_ce.go|_ce_test.go|_ent.go|_ent_test.go)
+      - linters:
+          - staticcheck
+        text: 'SA1019:'
+      - linters:
+          - staticcheck
+        text: 'ST1005: error strings should not end with punctuation or newlines'
+      - linters:
+          - staticcheck
+        text: 'ST1012: error var injectedError should have name of the form errFo'
+      - linters:
+          - staticcheck
+        text: 'ST1005: error strings should not be capitalized'
+      - linters:
+          - staticcheck
+        text: 'ST1012: error var'
+    paths:
+      - third_party$
+      - builtin$
+      - examples$
+      - agent/consul
+formatters:
+  enable:
+    - gofmt
+  settings:
+    gofmt:
+      simplify: true
+  exclusions:
+    generated: lax
+    paths:
+      - third_party$
+      - builtin$
+      - examples$

Makefile

@@ -10,7 +10,7 @@ GO_MODULES := $(shell find . -name go.mod -exec dirname {} \; | grep -v "proto-g
 # These version variables can either be a valid string for "go install <module>@<version>"
 # or the string @DEV to imply use what is currently installed locally.
 ###
-GOLANGCI_LINT_VERSION='v1.64.8'
+GOLANGCI_LINT_VERSION='v2.4.0'
 MOCKERY_VERSION='v3.5.2'
 BUF_VERSION='v1.26.0'
 

acl/acl_ce.go

@@ -32,6 +32,4 @@ type EnterpriseConfig struct {
 	// no fields in CE
 }
 
-func (_ *EnterpriseConfig) Close() {
-	// do nothing
-}
+func (*EnterpriseConfig) Close() {}

acl/enterprisemeta_ce.go

@@ -117,7 +117,7 @@ func NewEnterpriseMetaWithPartition(_, _ string) EnterpriseMeta {
 }
 
 // FillAuthzContext stub
-func (_ *EnterpriseMeta) FillAuthzContext(_ *AuthorizerContext) {}
+func (*EnterpriseMeta) FillAuthzContext(_ *AuthorizerContext) {}
 
 func NormalizeNamespace(_ string) string {
 	return ""

agent/acl_endpoint.go

@@ -26,10 +26,7 @@ var aclDisabled = HTTPError{StatusCode: http.StatusUnauthorized, Reason: "ACL su
 // checkACLDisabled will return a standard response if ACLs are disabled. This
 // returns true if they are disabled and we should not continue.
 func (s *HTTPHandlers) checkACLDisabled() bool {
-	if s.agent.config.ACLsEnabled {
-		return false
-	}
-	return true
+	return !(s.agent.config.ACLsEnabled)
 }
 
 // ACLBootstrap is used to perform a one-time ACL bootstrap operation on

agent/acl_endpoint_test.go

@@ -34,10 +34,7 @@ import (
 
 func isHTTPBadRequest(err error) bool {
 	if err, ok := err.(HTTPError); ok {
-		if err.StatusCode != 400 {
-			return false
-		}
-		return true
+		return err.StatusCode == http.StatusBadRequest
 	}
 	return false
 }

agent/agent.go

@@ -4216,10 +4216,8 @@ func revertStaticConfig(oldCfg tlsutil.ProtocolConfig, newCfg tlsutil.ProtocolCo
 	newOldcfg := newCfg
 	newOldcfg.CertFile = oldCfg.CertFile
 	newOldcfg.KeyFile = oldCfg.KeyFile
-	if !reflect.DeepEqual(newOldcfg, oldCfg) {
-		return true
-	}
-	return false
+
+	return !reflect.DeepEqual(newOldcfg, oldCfg)
 }
 
 // reloadConfigInternal is mainly needed for some unit tests. Instead of parsing

agent/checks/check_test.go

@@ -1198,11 +1198,10 @@ func mockUDPServer(ctx context.Context, network string, port int) {
 		}
 	}()
 
-	select {
-	case <-ctx.Done():
-		fmt.Println("cancelled")
-		close(chClose)
-	}
+	<-ctx.Done()
+	fmt.Println("cancelled")
+	close(chClose)
+
 	wg.Wait()
 }
 
@@ -1366,7 +1365,7 @@ func TestCheckH2PING(t *testing.T) {
 }
 
 func TestCheckH2PING_TLS_BadVerify(t *testing.T) {
-	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { return })
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
 	server := httptest.NewUnstartedServer(handler)
 	server.EnableHTTP2 = true
 	server.StartTLS()
@@ -1468,7 +1467,7 @@ func TestCheckH2CPING(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
-			handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { return })
+			handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {})
 			h2chandler := h2c.NewHandler(handler, &http2.Server{})
 			server := httptest.NewUnstartedServer(h2chandler)
 			server.Config.ReadTimeout = tt.connTimeout

agent/config/config_ce.go

@@ -13,6 +13,6 @@ import (
 // EnterpriseMeta stub
 type EnterpriseMeta struct{}
 
-func (_ *EnterpriseMeta) ToStructs() acl.EnterpriseMeta {
+func (*EnterpriseMeta) ToStructs() acl.EnterpriseMeta {
 	return *structs.DefaultEnterpriseMetaInDefaultPartition()
 }