Merge pull request #4847 from vincepri/linter

🌱 Align golangci-lint with upstream Cluster API

.github/workflows/pr-golangci-lint.yaml

@@ -0,0 +1,33 @@
+name: PR golangci-lint
+
+on:
+  pull_request:
+    types: [opened, edited, synchronize, reopened]
+
+# Remove all permissions from GITHUB_TOKEN except metadata.
+permissions: {}
+
+jobs:
+  golangci:
+    name: lint
+    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        working-directory:
+          - ""
+    steps:
+      - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # tag=v4.1.1
+      - name: Calculate go version
+        id: vars
+        run: echo "go_version=$(make go-version)" >> $GITHUB_OUTPUT
+      - name: Set up Go
+        uses: actions/setup-go@0c52d547c9bc32b1aa3301fd7a9cb496313a4491 # tag=v5.0.0
+        with:
+          go-version: ${{ steps.vars.outputs.go_version }}
+      - name: golangci-lint
+        uses: golangci/golangci-lint-action@3cfe3a4abbb849e10058ce4af15d205b6da42804 # tag=v4.0.0
+        with:
+          version: v1.56.1
+          args: --out-format=colored-line-number
+          working-directory: ${{matrix.working-directory}}

.golangci.yml

@@ -1,79 +1,126 @@
 linters:
-  enable-all: true
-  disable:
-    - bidichk
-    - contextcheck
-    - cyclop
-    - dupl
-    - durationcheck
-    - errname
-    - errorlint
-    - exhaustive
-    - exhaustivestruct
-    - exhaustruct
-    - forcetypeassert
-    - forbidigo
-    - funlen
-    - gochecknoglobals
-    - gochecknoinits
-    - gocognit
-    - godox
-    - goerr113
-    - gofumpt
-    - golint
-    - gomnd
-    - gomoddirectives
-    - gomodguard
-    - interfacer
-    - ireturn
-    - lll
-    - makezero
-    - maligned
-    - musttag
-    - nestif
-    - nilnil
-    - nlreturn
-    - nonamedreturns
-    - nosnakecase
-    - paralleltest
-    - promlinter
-    - scopelint
-    - sqlclosecheck
-    - tagliatelle
-    - tenv
-    - testpackage
-    - tparallel
-    - varnamelen
-    - wastedassign
-    - wrapcheck
-    - wsl
-    - deadcode
-    - ifshort
-    - structcheck
-    - varcheck
-    - interfacebloat
+  disable-all: true
+  enable:
+  - asasalint
+  - asciicheck
+  - bidichk
+  - bodyclose
+  - containedctx
+  - dogsled
+  - dupword
+  - durationcheck
+  - errcheck
+  - errchkjson
+  - exportloopref
+  - gci
+  - ginkgolinter
+  - goconst
+  - gocritic
+  - godot
+  - gofmt
+  - goimports
+  - goprintffuncname
+  - gosec
+  - gosimple
+  - govet
+  - importas
+  - ineffassign
+  - loggercheck
+  - misspell
+  - nakedret
+  - nilerr
+  - noctx
+  - nolintlint
+  - nosprintfhostport
+  - prealloc
+  - predeclared
+  - revive
+  - rowserrcheck
+  - staticcheck
+  - stylecheck
+  - thelper
+  - typecheck
+  - unconvert
+  - unparam
+  - unused
+  - usestdlibvars
+  - whitespace
 
 linters-settings:
-  # Restrict revive to exported.
-  revive:
-    # see https://github.com/mgechev/revive#available-rules for details.
-    ignore-generated-header: true
-    severity: warning
-    rules:
-      - name: exported
-        severity: warning
   gci:
     sections:
       - standard
       - default
       - prefix(sigs.k8s.io/cluster-api)
   ginkgolinter:
-    # Suppress the wrong length assertion warning.
-    suppress-len-assertion: true
-    # Suppress the wrong nil assertion warning.
-    suppress-nil-assertion: false
-    # Suppress the wrong error assertion warning.
-    suppress-err-assertion: true
+    forbid-focus-container: true
+    suppress-len-assertion: true # Suppress the wrong length assertion warning.
+    suppress-nil-assertion: false # Suppress the wrong nil assertion warning.
+    suppress-err-assertion: true # Suppress the wrong error assertion warning.
+  gocritic:
+    enabled-tags:
+      - diagnostic
+      - experimental
+      - performance
+    disabled-checks:
+      - appendAssign
+      - dupImport # https://github.com/go-critic/go-critic/issues/845
+      - evalOrder
+      - ifElseChain
+      - octalLiteral
+      - regexpSimplify
+      - sloppyReassign
+      - truncateCmp
+      - typeDefFirst
+      - unnamedResult
+      - unnecessaryDefer
+      - whyNoLint
+      - wrapperFunc
+      - rangeValCopy
+      - hugeParam
+      - filepathJoin
+      - emptyStringTest
+  godot:
+    #   declarations - for top level declaration comments (default);
+    #   toplevel     - for top level comments;
+    #   all          - for all comments.
+    scope: toplevel
+    exclude:
+      - '^ \+.*'
+      - '^ ANCHOR.*'
+  revive:
+    rules:
+      # The following rules are recommended https://github.com/mgechev/revive#recommended-configuration
+      - name: blank-imports
+      - name: context-as-argument
+      - name: context-keys-type
+      - name: dot-imports
+      - name: error-return
+      - name: error-strings
+      - name: error-naming
+      - name: exported
+      - name: if-return
+      - name: increment-decrement
+      - name: var-naming
+      - name: var-declaration
+      - name: package-comments
+      - name: range
+      - name: receiver-naming
+      - name: time-naming
+      - name: unexported-return
+      - name: indent-error-flow
+      - name: errorf
+      - name: empty-block
+      - name: superfluous-else
+      - name: unreachable-code
+      - name: redefines-builtin-id
+      #
+      # Rules in addition to the recommended configuration above.
+      #
+      - name: bool-literal-in-expr
+      - name: constant-logical-expr
+  goconst:
+    ignore-tests: true
   gosec:
     excludes:
       - G307 # Deferring unsafe method "Close" on type "\*os.File"
@@ -159,6 +206,10 @@ linters-settings:
         alias: apimachinerytypes
       - pkg: "sigs.k8s.io/cluster-api/exp/api/v1beta1"
         alias: expclusterv1
+  nolintlint:
+    allow-unused: false
+    allow-leading-space: false
+    require-specific: true
   staticcheck:
     go: "1.21"
   stylecheck:
@@ -178,7 +229,6 @@ issues:
   # List of regexps of issue texts to exclude, empty list by default.
   exclude:
     - (Expect directory permissions to be 0750 or less|Expect file permissions to be 0600 or less)
-    - "exported: exported (const|function|method|type|var) (.+) should have comment or be unexported"
     - "exported: (func|type) name will be used as (.+) by other packages, and that stutters; consider calling this (.+)"
     - (G104|G107|G404|G505|ST1000)
     - "G108: Profiling endpoint is automatically exposed on /debug/pprof"
@@ -188,6 +238,13 @@ issues:
     - "net/http.Get must not be called"
   exclude-rules:
     # Exclude revive's exported for certain packages and code, e.g. tests and fake.
+    - linters:
+      - revive
+      text: "exported: exported method .*\\.(Reconcile|SetupWithManager|SetupWebhookWithManager) should have comment or be unexported"
+    - linters:
+      - errcheck
+      text: Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked
+    # Exclude some packages or code to require comments, for example test code, or fake clients.
     - linters:
       - revive
       text: exported (method|function|type|const) (.+) should have comment or be unexported
@@ -229,6 +286,11 @@ issues:
       - revive
       text: "var-naming: don't use underscores in Go names; func (.+) should be (.+)"
       path: .*/defaults.go
+    # These directives allow the mock and gc packages to be imported with an underscore everywhere.
+    - linters:
+      - revive
+      text: "var-naming: don't use an underscore in package name"
+      path: .*/.*(mock|gc_).*/.+\.go
     # Disable unparam "always receives" which might not be really
     # useful when building libraries.
     - linters:

Makefile

@@ -48,6 +48,7 @@ KUBETEST_CONF_PATH ?= $(abspath $(E2E_DATA_DIR)/kubetest/conformance.yaml)
 EXP_DIR := exp
 
 # Binaries.
+GO_INSTALL := ./scripts/go_install.sh
 GO_APIDIFF_BIN := $(BIN_DIR)/go-apidiff
 GO_APIDIFF := $(TOOLS_DIR)/$(GO_APIDIFF_BIN)
 CLUSTERCTL := $(BIN_DIR)/clusterctl
@@ -58,7 +59,10 @@ DEFAULTER_GEN := $(TOOLS_BIN_DIR)/defaulter-gen
 ENVSUBST := $(TOOLS_BIN_DIR)/envsubst
 GH := $(TOOLS_BIN_DIR)/gh
 GOJQ := $(TOOLS_BIN_DIR)/gojq
-GOLANGCI_LINT := $(TOOLS_BIN_DIR)/golangci-lint
+GOLANGCI_LINT_BIN := golangci-lint
+GOLANGCI_LINT_VER := $(shell cat .github/workflows/pr-golangci-lint.yaml | grep [[:space:]]version: | sed 's/.*version: //')
+GOLANGCI_LINT := $(abspath $(TOOLS_BIN_DIR)/$(GOLANGCI_LINT_BIN)-$(GOLANGCI_LINT_VER))
+GOLANGCI_LINT_PKG := github.com/golangci/golangci-lint/cmd/golangci-lint
 KIND := $(TOOLS_BIN_DIR)/kind
 KUSTOMIZE := $(TOOLS_BIN_DIR)/kustomize
 MOCKGEN := $(TOOLS_BIN_DIR)/mockgen
@@ -290,6 +294,9 @@ generate-go-apis: ## Alias for .build/generate-go-apis
 
 .PHONY: modules
 
+$(GOLANGCI_LINT): # Build golangci-lint from tools folder.
+	GOBIN=$(abspath $(TOOLS_BIN_DIR)) $(GO_INSTALL) $(GOLANGCI_LINT_PKG) $(GOLANGCI_LINT_BIN) $(GOLANGCI_LINT_VER)
+
 .PHONY: lint
 lint: $(GOLANGCI_LINT) ## Lint codebase
 	$(GOLANGCI_LINT) run -v --fast=false $(GOLANGCI_LINT_EXTRA_ARGS)

api/v1beta1/awscluster_types.go

@@ -207,6 +207,7 @@ type AWSClusterStatus struct {
 	Conditions     clusterv1.Conditions     `json:"conditions,omitempty"`
 }
 
+// S3Bucket defines a supporting S3 bucket for the cluster, currently can be optionally used for Ignition.
 type S3Bucket struct {
 	// ControlPlaneIAMInstanceProfile is a name of the IAMInstanceProfile, which will be allowed
 	// to read control-plane node bootstrap data from S3 Bucket.

api/v1beta1/awsclustertemplate_types.go

@@ -53,6 +53,7 @@ func init() {
 	SchemeBuilder.Register(&AWSClusterTemplate{}, &AWSClusterTemplateList{})
 }
 
+// AWSClusterTemplateResource defines the desired state of AWSClusterTemplate.
 type AWSClusterTemplateResource struct {
 	// Standard object's metadata.
 	// More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata

api/v1beta1/conversion_test.go

@@ -19,9 +19,8 @@ package v1beta1
 import (
 	"testing"
 
-	. "github.com/onsi/gomega"
-
 	fuzz "github.com/google/gofuzz"
+	. "github.com/onsi/gomega"
 	"k8s.io/apimachinery/pkg/api/apitesting/fuzzer"
 	"k8s.io/apimachinery/pkg/runtime"
 	runtimeserializer "k8s.io/apimachinery/pkg/runtime/serializer"
@@ -38,7 +37,7 @@ func fuzzFuncs(_ runtimeserializer.CodecFactory) []interface{} {
 
 func AWSMachineFuzzer(obj *AWSMachine, c fuzz.Continue) {
 	c.FuzzNoCustom(obj)
-	
+
 	// AWSMachine.Spec.FailureDomain, AWSMachine.Spec.Subnet.ARN and AWSMachine.Spec.AdditionalSecurityGroups.ARN has been removed in v1beta2, so setting it to nil in order to avoid v1beta1 --> v1beta2 --> v1beta1 round trip errors.
 	if obj.Spec.Subnet != nil {
 		obj.Spec.Subnet.ARN = nil
@@ -54,7 +53,7 @@ func AWSMachineFuzzer(obj *AWSMachine, c fuzz.Continue) {
 
 func AWSMachineTemplateFuzzer(obj *AWSMachineTemplate, c fuzz.Continue) {
 	c.FuzzNoCustom(obj)
-	
+
 	// AWSMachineTemplate.Spec.Template.Spec.FailureDomain, AWSMachineTemplate.Spec.Template.Spec.Subnet.ARN and AWSMachineTemplate.Spec.Template.Spec.AdditionalSecurityGroups.ARN has been removed in v1beta2, so setting it to nil in order to avoid  v1beta1 --> v1beta2 --> v1beta round trip errors.
 	if obj.Spec.Template.Spec.Subnet != nil {
 		obj.Spec.Template.Spec.Subnet.ARN = nil
@@ -81,16 +80,16 @@ func TestFuzzyConversion(t *testing.T) {
 	}))
 
 	t.Run("for AWSMachine", utilconversion.FuzzTestFunc(utilconversion.FuzzTestFuncInput{
-		Scheme: scheme,
-		Hub:    &v1beta2.AWSMachine{},
-		Spoke:  &AWSMachine{},
+		Scheme:      scheme,
+		Hub:         &v1beta2.AWSMachine{},
+		Spoke:       &AWSMachine{},
 		FuzzerFuncs: []fuzzer.FuzzerFuncs{fuzzFuncs},
 	}))
 
 	t.Run("for AWSMachineTemplate", utilconversion.FuzzTestFunc(utilconversion.FuzzTestFuncInput{
-		Scheme: scheme,
-		Hub:    &v1beta2.AWSMachineTemplate{},
-		Spoke:  &AWSMachineTemplate{},
+		Scheme:      scheme,
+		Hub:         &v1beta2.AWSMachineTemplate{},
+		Spoke:       &AWSMachineTemplate{},
 		FuzzerFuncs: []fuzzer.FuzzerFuncs{fuzzFuncs},
 	}))