diff --git a/.golangci.yml b/.golangci.yml index bf186a332d2..65b0fb1fa51 100644 --- a/.golangci.yml +++ b/.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,7 +255,7 @@ formatters: gofumpt: extra-rules: true golines: - max-len: 100 + max-len: 500 tab-len: 4 shorten-comments: true enable: @@ -228,6 +263,6 @@ formatters: - gofmt # We might consider enabling the following: # - gofumpt - # - golines + - golines exclusions: generated: disable diff --git a/cmd/nerdctl/container/container_update.go b/cmd/nerdctl/container/container_update.go index fb728655d29..28ac0f6d078 100644 --- a/cmd/nerdctl/container/container_update.go +++ b/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 --git a/cmd/nerdctl/inspect/inspect.go b/cmd/nerdctl/inspect/inspect.go index 53d02858d5a..8c62f54e4d1 100644 --- a/cmd/nerdctl/inspect/inspect.go +++ b/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 --git a/pkg/api/types/image_types.go b/pkg/api/types/image_types.go index 0a723ca2cd2..d48e6318026 100644 --- a/pkg/api/types/image_types.go +++ b/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`. diff --git a/pkg/cmd/container/run_windows.go b/pkg/cmd/container/run_windows.go index 6b38e7e9f3f..6bc5b42e699 100644 --- a/pkg/cmd/container/run_windows.go +++ b/pkg/cmd/container/run_windows.go @@ -69,17 +69,17 @@ func setPlatformOptions( if err != nil { return nil, fmt.Errorf("failed to parse memory bytes %q: %w", options.Memory, err) } - UVMMemmory := map[string]string{ + uvmMemmory := map[string]string{ uvmMemorySizeInMB: fmt.Sprintf("%v", mem64), } - opts = append(opts, oci.WithAnnotations(UVMMemmory)) + opts = append(opts, oci.WithAnnotations(uvmMemmory)) } if options.CPUs > 0.0 { - UVMCPU := map[string]string{ + uvmCPU := map[string]string{ uvmCPUCount: fmt.Sprintf("%v", options.CPUs), } - opts = append(opts, oci.WithAnnotations(UVMCPU)) + opts = append(opts, oci.WithAnnotations(uvmCPU)) } opts = append(opts, oci.WithWindowsHyperV) case "host": diff --git a/pkg/cmd/network/list.go b/pkg/cmd/network/list.go index b2075356d8c..731c51b1b99 100644 --- a/pkg/cmd/network/list.go +++ b/pkg/cmd/network/list.go @@ -35,7 +35,7 @@ type networkPrintable struct { Name string Labels string // TODO: "CreatedAt", "Driver", "IPv6", "Internal", "Scope" - file string `json:"-"` + file string } func List(ctx context.Context, options types.NetworkListOptions) error { diff --git a/pkg/containerutil/container_network_manager.go b/pkg/containerutil/container_network_manager.go index c16ee9dfb9a..9f082f9ce12 100644 --- a/pkg/containerutil/container_network_manager.go +++ b/pkg/containerutil/container_network_manager.go @@ -240,11 +240,7 @@ func (m *noneNetworkManager) SetupNetworking(ctx context.Context, containerID st } // Save the meta information - if err = hs.Acquire(hsMeta); err != nil { - return err - } - - return nil + return hs.Acquire(hsMeta) } // CleanupNetworking Performs any required cleanup actions for the given container. @@ -269,10 +265,7 @@ func (m *noneNetworkManager) CleanupNetworking(ctx context.Context, container co } // Release - if err = hs.Release(container.ID()); err != nil { - return err - } - return nil + return hs.Release(container.ID()) } // InternalNetworkingOptionLabels Returns the set of NetworkingOptions which should be set as labels on the container. diff --git a/pkg/dnsutil/hostsstore/hosts.go b/pkg/dnsutil/hostsstore/hosts.go index 1b25b02d765..1378ed39686 100644 --- a/pkg/dnsutil/hostsstore/hosts.go +++ b/pkg/dnsutil/hostsstore/hosts.go @@ -52,7 +52,7 @@ func parseHostsButSkipMarkedRegion(w io.Writer, r io.Reader) error { LINE: for scanner.Scan() { line := scanner.Text() - line = strings.Replace(strings.Trim(line, " \t"), "\t", " ", -1) + line = strings.ReplaceAll(strings.Trim(line, " \t"), "\t", " ") sawMarkerEnd := false if strings.HasPrefix(line, "#") { com := strings.TrimSpace(line[1:]) diff --git a/pkg/imgutil/commit/commit.go b/pkg/imgutil/commit/commit.go index 782a5cd6edb..fd5886bfb7f 100644 --- a/pkg/imgutil/commit/commit.go +++ b/pkg/imgutil/commit/commit.go @@ -48,7 +48,7 @@ import ( "github.com/containerd/nerdctl/v2/pkg/clientutil" "github.com/containerd/nerdctl/v2/pkg/cmd/image" "github.com/containerd/nerdctl/v2/pkg/containerutil" - imgutil "github.com/containerd/nerdctl/v2/pkg/imgutil" + "github.com/containerd/nerdctl/v2/pkg/imgutil" "github.com/containerd/nerdctl/v2/pkg/labels" ) diff --git a/pkg/imgutil/filtering.go b/pkg/imgutil/filtering.go index 26525d86cb2..30764f163c6 100644 --- a/pkg/imgutil/filtering.go +++ b/pkg/imgutil/filtering.go @@ -158,8 +158,7 @@ func FilterByCreatedAt(ctx context.Context, client *containerd.Client, before [] return []images.Image{}, err } if len(beforeImages) == 0 { - //nolint:stylecheck - return []images.Image{}, fmt.Errorf("No such image: %s", fetchImageNames(before)) + return []images.Image{}, fmt.Errorf("no such image: %s", fetchImageNames(before)) } maxTime = beforeImages[0].CreatedAt for _, image := range beforeImages { @@ -175,8 +174,7 @@ func FilterByCreatedAt(ctx context.Context, client *containerd.Client, before [] return []images.Image{}, err } if len(sinceImages) == 0 { - //nolint:stylecheck - return []images.Image{}, fmt.Errorf("No such image: %s", fetchImageNames(since)) + return []images.Image{}, fmt.Errorf("no such image: %s", fetchImageNames(since)) } minTime = sinceImages[0].CreatedAt for _, image := range sinceImages { diff --git a/pkg/infoutil/infoutilmock/info.util.mock.go b/pkg/infoutil/infoutilmock/infoutil_mock.go similarity index 100% rename from pkg/infoutil/infoutilmock/info.util.mock.go rename to pkg/infoutil/infoutilmock/infoutil_mock.go diff --git a/pkg/inspecttypes/dockercompat/dockercompat.go b/pkg/inspecttypes/dockercompat/dockercompat.go index 4605de3d1e7..e5b797e786d 100644 --- a/pkg/inspecttypes/dockercompat/dockercompat.go +++ b/pkg/inspecttypes/dockercompat/dockercompat.go @@ -820,10 +820,10 @@ func getMemorySettingsFromNative(sp *specs.Spec) (*MemorySetting, error) { return res, nil } -func getDNSFromNative(Labels map[string]string) (*DNSSettings, error) { +func getDNSFromNative(lbls map[string]string) (*DNSSettings, error) { res := &DNSSettings{} - if dnsSettingJSON, ok := Labels[labels.DNSSetting]; ok { + if dnsSettingJSON, ok := lbls[labels.DNSSetting]; ok { if err := json.Unmarshal([]byte(dnsSettingJSON), &res); err != nil { return nil, fmt.Errorf("failed to parse DNS settings: %v", err) } @@ -832,10 +832,10 @@ func getDNSFromNative(Labels map[string]string) (*DNSSettings, error) { return res, nil } -func getHostConfigLabelFromNative(Labels map[string]string) (*HostConfigLabel, error) { +func getHostConfigLabelFromNative(lbls map[string]string) (*HostConfigLabel, error) { res := &HostConfigLabel{} - if hostConfigLabelJSON, ok := Labels[labels.HostConfigLabel]; ok { + if hostConfigLabelJSON, ok := lbls[labels.HostConfigLabel]; ok { if err := json.Unmarshal([]byte(hostConfigLabelJSON), &res); err != nil { return nil, fmt.Errorf("failed to parse DNS servers: %v", err) } diff --git a/pkg/netutil/netutil_test.go b/pkg/netutil/netutil_test.go index a17f4cb6197..399a33ee436 100644 --- a/pkg/netutil/netutil_test.go +++ b/pkg/netutil/netutil_test.go @@ -198,7 +198,7 @@ func testDefaultNetworkCreation(t *testing.T) { assert.NilError(t, err) assert.Assert(t, len(files) == 2) // files[0] is the entry for '.' assert.Assert(t, filepath.Join(cniConfTestDir, files[1].Name()) == defaultNetConf.File) - assert.Assert(t, firstConfigModTime == files[1].ModTime()) + assert.Assert(t, firstConfigModTime.Equal(files[1].ModTime())) } // Tests whether nerdctl properly creates the default network @@ -297,7 +297,7 @@ func testDefaultNetworkCreationWithBridgeIP(t *testing.T) { assert.NilError(t, err) assert.Assert(t, len(files) == 2) // files[0] is the entry for '.' assert.Assert(t, filepath.Join(cniConfTestDir, files[1].Name()) == defaultNetConf.File) - assert.Assert(t, firstConfigModTime == files[1].ModTime()) + assert.Assert(t, firstConfigModTime.Equal(files[1].ModTime())) } // Tests whether nerdctl skips the creation of the default network if a