Merge pull request #4051 from apostasie/ci-golangci-v2-post

[CI] golangci v2: review, fix, document, rules

Author: AkihiroSuda

Diff for: .golangci.yml

@@ -10,16 +10,36 @@ issues:
 linters:
   default: none
   enable:
-    - depguard
+    # 1. This is the default enabled set of golanci
+
+    # We should consider enabling errcheck
+    # - errcheck
     - govet
     - ineffassign
-    - misspell
-    - nakedret
-    - prealloc
-    - revive
     - staticcheck
-    - unconvert
     - unused
+
+    # 2. These are not part of the default set
+
+    # Important to prevent import of certain packages
+    - depguard
+    # Removes unnecessary conversions
+    - unconvert
+    # Flag common typos
+    - misspell
+    # A meta-linter seen as a good replacement for golint
+    - revive
+    # Gocritic
+    - gocritic
+
+    # 3. We used to use these, but have now removed them
+
+    # Use of prealloc is generally premature optimization and performance profiling should be done instead
+    # https://golangci-lint.run/usage/linters/#prealloc
+    # - prealloc
+    # Provided by revive in a better way
+    # - nakedret
+
   settings:
     staticcheck:
       checks:
@@ -31,93 +51,122 @@ linters:
         - "-ST1020"
         - "-ST1021"
         - "-ST1022"
-        # FIXME: this below this point is disabled for now, but we should investigate
-        - "-QF1008"  # Omit embedded fields from selector expression https://staticcheck.dev/docs/checks#QF1008
-        - "-QF1003"  # Convert if/else-if chain to tagged switch https://staticcheck.dev/docs/checks#QF1003
-        - "-QF1009"  # Use time.Time.Equal instead of == operator https://staticcheck.dev/docs/checks#QF1009
-        - "-QF1001"  # Apply De Morgan’s law https://staticcheck.dev/docs/checks#QF1001
-        - "-QF1012"  # Use fmt.Fprintf(x, ...) instead of x.Write(fmt.Sprintf(...)) https://staticcheck.dev/docs/checks#QF1012
-        - "-ST1005"  # Expand call to math.Pow https://staticcheck.dev/docs/checks#QF1005
-        - "-QF1004"  # Use strings.ReplaceAll instead of strings.Replace with n == -1 https://staticcheck.dev/docs/checks#QF1004
+
+        ##### TODO: fix and enable these
+        # 4 occurrences.
+        # Use fmt.Fprintf(x, ...) instead of x.Write(fmt.Sprintf(...)) https://staticcheck.dev/docs/checks#QF1012
+        - "-QF1012"
+        # 6 occurrences.
+        # Apply De Morgan’s law https://staticcheck.dev/docs/checks#QF1001
+        - "-QF1001"
+        # 10 occurrences.
+        # Convert if/else-if chain to tagged switch https://staticcheck.dev/docs/checks#QF1003
+        - "-QF1003"
+
+        ##### These have been vetted to be disabled.
+        # 55 occurrences. Omit embedded fields from selector expression https://staticcheck.dev/docs/checks#QF1008
+        # Usefulness is questionable.
+        - "-QF1008"
+
     revive:
       enable-all-rules: true
       rules:
         # See https://revive.run/r
-        # Below are unsorted, and we might want to review them to decide which one we want to enable
-        - name: exported
+
+        ##### P0: we should do it ASAP.
+        - name: max-control-nesting
+          # 10 occurences (at default 5). Deep nesting hurts readibility.
+          arguments: [7]
+        - name: deep-exit
+          # 11 occurrences. Do not exit in random places.
           disabled: true
-        - name: add-constant
+        - name: unchecked-type-assertion
+          # 14 occurrences. This is generally risky and encourages bad coding for newcomers.
           disabled: true
-        - name: cognitive-complexity
+        - name: bare-return
+          # 31 occurrences. Bare returns are just evil, very unfriendly, and make reading and editing much harder.
           disabled: true
-        - name: package-comments
+        - name: import-shadowing
+          # 44 occurrences. Shadowing makes things prone to errors / confusing to read.
           disabled: true
-        - name: cyclomatic
+        - name: use-errors-new
+          # 84 occurrences. Improves error testing.
           disabled: true
-        - name: deep-exit
+
+        ##### P1: consider making a dent on these, but not critical.
+        - name: argument-limit
+          # 4 occurrences (at default 8). Long windy arguments list for functions are hard to read. Use structs instead.
+          arguments: [12]
+        - name: unnecessary-stmt
+          # 5 occurrences. Increase readability.
           disabled: true
-        - name: function-length
+        - name: defer
+          # 7 occurrences. Confusing to read for newbies.
           disabled: true
-        - name: flag-parameter
+        - name: confusing-naming
+          # 10 occurrences. Hurts readability.
           disabled: true
-        - name: max-public-structs
+        - name: early-return
+          # 10 occurrences. Would improve readability.
           disabled: true
-        - name: max-control-nesting
+        - name: function-result-limit
+          # 12 occurrences (at default 3). A function returning many results is probably too big.
+          arguments: [7]
+        - name: function-length
+          # 155 occurrences (at default 0, 75). Really long functions should really be broken up in most cases.
+          arguments: [0, 400]
+        - name: cyclomatic
+          # 204 occurrences (at default 10)
+          arguments: [100]
+        - name: unhandled-error
+          # 222 occurrences. Could indicate failure to handle broken conditions.
           disabled: true
+        - name: cognitive-complexity
+          arguments: [197]
+          # 441 occurrences (at default 7). We should try to lower it (involves significant refactoring).
+
+        ##### P2: nice to have.
+        - name: max-public-structs
+          # 7 occurrences (at default 5). Might indicate overcrowding of public API.
+          arguments: [21]
         - name: confusing-results
+          # 13 occurrences. Have named returns when the type stutters.
+          # Makes it a bit easier to figure out function behavior just looking at signature.
           disabled: true
-        - name: nested-structs
-          disabled: true
-        - name: import-alias-naming
-          disabled: true
-        - name: filename-format
+        - name: comment-spacings
+          # 50 occurrences. Makes code look less wonky / ease readability.
           disabled: true
         - name: use-any
-          disabled: true
-        # FIXME: we should enable these below
-        - name: struct-tag
-          disabled: true
-        - name: redundant-import-alias
+          # 30 occurrences. `any` instead of `interface{}`. Cosmetic.
           disabled: true
         - name: empty-lines
+          # 85 occurrences. Makes code look less wonky / ease readability.
           disabled: true
-        - name: unhandled-error
-          disabled: true
-        - name: confusing-naming
-          disabled: true
-        - name: unused-parameter
-          disabled: true
-        - name: unused-receiver
-          disabled: true
-        - name: import-shadowing
-          disabled: true
-        - name: use-errors-new
-          disabled: true
-        - name: argument-limit
-          disabled: true
-        - name: time-equal
-          disabled: true
-        - name: defer
-          disabled: true
-        - name: early-return
-          disabled: true
-        - name: comment-spacings
+        - name: package-comments
+          # 100 occurrences. Better for documentation...
           disabled: true
-        - name: function-result-limit
+        - name: exported
+          # 577 occurrences. Forces documentation of any exported symbol.
           disabled: true
-        - name: unexported-naming
+
+        ###### Permanently disabled. Below have been reviewed and vetted to be unnecessary.
+        - name: line-length-limit
+          # Formatter `golines` takes care of this.
           disabled: true
-        - name: unnecessary-stmt
+        - name: nested-structs
+          # 5 occurrences. Trivial. This is not that hard to read.
           disabled: true
-        - name: if-return
+        - name: flag-parameter
+          # 52 occurrences. Not sure if this is valuable.
           disabled: true
-        - name: unchecked-type-assertion
+        - name: unused-parameter
+          # 505 occurrences. A lot of work for a marginal improvement.
           disabled: true
-        - name: bare-return
+        - name: unused-receiver
+          # 31 occurrences. Ibid.
           disabled: true
-        # Below have been reviewed and disabled
-        - name: line-length-limit
-          # Better dealt with by formatter golines
+        - name: add-constant
+          # 2605 occurrences. Kind of useful in itself, but unacceptable amount of effort to fix
           disabled: true
 
     depguard:
@@ -141,67 +190,53 @@ linters:
             - pkg: github.com/containerd/nerdctl/v2/cmd
               desc: pkg must not depend on any cmd files
     gocritic:
-      enabled-checks:
+      disabled-checks:
+        # Below are normally enabled by default, but we do not pass
         - appendAssign
-        - argOrder
-        - badCond
-        - caseOrder
-        - codegenComment
-        - commentedOutCode
-        - deprecatedComment
-        - dupArg
-        - dupBranchBody
-        - dupCase
-        - dupSubExpr
-        - exitAfterDefer
-        - flagDeref
-        - flagName
+        - ifElseChain
+        - unslice
+        - badCall
+        - assignOp
+        - commentFormatting
+        - captLocal
+        - singleCaseSwitch
+        - wrapperFunc
+        - elseif
+        - regexpMust
+      enabled-checks:
+        # Below used to be enabled, but we do not pass anymore
+        # - paramTypeCombine
+        # - octalLiteral
+        # - unnamedResult
+        # - equalFold
+        # - sloppyReassign
+        # - emptyStringTest
+        # - hugeParam
+        # - appendCombine
+        # - stringXbytes
+        # - ptrToRefParam
+        # - commentedOutCode
+        # - rangeValCopy
+        # - methodExprCall
+        # - yodaStyleExpr
+        # - typeUnparen
+
+        # We enabled these and we pass
         - nilValReturn
-        - offBy1
-        - sloppyReassign
         - weakCond
-        - octalLiteral
-        - appendCombine
-        - equalFold
-        - hugeParam
         - indexAlloc
         - rangeExprCopy
-        - rangeValCopy
-        - assignOp
         - boolExprSimplify
-        - captLocal
-        - commentFormatting
         - commentedOutImport
-        - defaultCaseOrder
         - docStub
-        - elseif
         - emptyFallthrough
-        - emptyStringTest
         - hexLiteral
-        - ifElseChain
-        - methodExprCall
-        - regexpMust
-        - singleCaseSwitch
-        - sloppyLen
-        - stringXbytes
-        - switchTrue
         - typeAssertChain
-        - typeSwitchVar
-        - underef
         - unlabelStmt
-        - unlambda
-        - unslice
-        - valSwap
-        - wrapperFunc
-        - yodaStyleExpr
         - builtinShadow
         - importShadow
         - initClause
         - nestingReduce
-        - paramTypeCombine
-        - ptrToRefParam
-        - typeUnparen
-        - unnamedResult
         - unnecessaryBlock
   exclusions:
     generated: disable
@@ -220,14 +255,14 @@ formatters:
     gofumpt:
       extra-rules: true
     golines:
-      max-len: 100
+      max-len: 500
       tab-len: 4
       shorten-comments: true
   enable:
     - gci
     - gofmt
     # We might consider enabling the following:
     #    - gofumpt
-    #    - golines
+    - golines
   exclusions:
     generated: disable

Diff for: cmd/nerdctl/container/container_update.go

@@ -38,7 +38,7 @@ import (
 	"github.com/containerd/nerdctl/v2/cmd/nerdctl/helpers"
 	"github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/containerd/nerdctl/v2/pkg/clientutil"
-	nerdctlContainer "github.com/containerd/nerdctl/v2/pkg/cmd/container"
+	nerdctlcontainer "github.com/containerd/nerdctl/v2/pkg/cmd/container"
 	"github.com/containerd/nerdctl/v2/pkg/formatter"
 	"github.com/containerd/nerdctl/v2/pkg/idutil/containerwalker"
 	"github.com/containerd/nerdctl/v2/pkg/infoutil"
@@ -358,7 +358,7 @@ func updateContainer(ctx context.Context, client *containerd.Client, id string,
 		return err
 	}
 	if cmd.Flags().Changed("restart") && restart != "" {
-		if err := nerdctlContainer.UpdateContainerRestartPolicyLabel(ctx, client, container, restart); err != nil {
+		if err := nerdctlcontainer.UpdateContainerRestartPolicyLabel(ctx, client, container, restart); err != nil {
 			return err
 		}
 	}

Diff for: cmd/nerdctl/inspect/inspect.go

@@ -23,9 +23,9 @@ import (
 	"github.com/spf13/cobra"
 
 	"github.com/containerd/nerdctl/v2/cmd/nerdctl/completion"
-	containerCmd "github.com/containerd/nerdctl/v2/cmd/nerdctl/container"
+	containercmd "github.com/containerd/nerdctl/v2/cmd/nerdctl/container"
 	"github.com/containerd/nerdctl/v2/cmd/nerdctl/helpers"
-	imageCmd "github.com/containerd/nerdctl/v2/cmd/nerdctl/image"
+	imagecmd "github.com/containerd/nerdctl/v2/cmd/nerdctl/image"
 	"github.com/containerd/nerdctl/v2/pkg/api/types"
 	"github.com/containerd/nerdctl/v2/pkg/clientutil"
 	"github.com/containerd/nerdctl/v2/pkg/cmd/container"
@@ -117,13 +117,13 @@ func inspectAction(cmd *cobra.Command, args []string) error {
 	var containerInspectOptions types.ContainerInspectOptions
 	if inspectImage {
 		platform := ""
-		imageInspectOptions, err = imageCmd.InspectOptions(cmd, &platform)
+		imageInspectOptions, err = imagecmd.InspectOptions(cmd, &platform)
 		if err != nil {
 			return err
 		}
 	}
 	if inspectContainer {
-		containerInspectOptions, err = containerCmd.InspectOptions(cmd)
+		containerInspectOptions, err = containercmd.InspectOptions(cmd)
 		if err != nil {
 			return err
 		}

Diff for: pkg/api/types/image_types.go

@@ -19,7 +19,7 @@ package types
 import (
 	"io"
 
-	v1 "github.com/opencontainers/image-spec/specs-go/v1"
+	"github.com/opencontainers/image-spec/specs-go/v1"
 )
 
 // ImageListOptions specifies options for `nerdctl image list`.