feat(golangcilint): bump to v2

[fredrikaverpil]

Why?

Golangci-lint was bumped to v2.

What?

• Specify v2.0.2 in Sage.
• Run golangci-lint migrate -c golangci.yml. This sorted the linters for us, so they are now in alphabetical order. The gci, gofumpt linters were removed (now exists as formatters) and gosimple has been absorbed by staticcheck.
• Add comments back in which were removed by the migration tool.
• Address issues detected:

   Error: [go-lint] ../../../internal/codegen/imports.go:102:10: QF1001: could apply De Morgan's law (staticcheck)
   [go-lint] 		return !('a' <= r && r <= 'z' || 'A' <= r && r <= 'Z' || '0' <= r && r <= '9' || r == '_' ||
   [go-lint] 		       ^
   Error: [go-lint] ../../../tools/sgartifactregistry/auth.go:60:2: QF1002: could use tagged switch on yarnMajor (staticcheck)
   [go-lint] 	switch {
   [go-lint] 	^
   [go-lint] 2 issues:
  

• Had to set the relative-path-mode (see separate comment).
• Removed the --exclude "(is a global variable|is unused)" CLI argument (as it must now be configured in the yaml instead). But I don't think this kind of exclusion is needed, so I did not add it into the yaml.

Will be saved for a separate PR

• Formatting capabilities: feat(golangcilint): add formatting capabilities #662
• Discussing adding/removing/reconfiguring linters - can be saved for backend guild session.

Enabled vs disabled linters

Name	Description	Enabled
asasalint	Check for pass []any as any in variadic func(...any).	✅
asciicheck	Checks that all code identifiers does not have non-ASCII symbols in the name.	✅
bidichk	Checks for dangerous unicode character sequences.	✅
bodyclose	Checks whether HTTP response body is closed successfully.	✅
canonicalheader	Canonicalheader checks whether net/http.Header uses canonical header.
containedctx	Containedctx is a linter that detects struct contained context.Context field.
contextcheck	Check whether the function uses a non-inherited context.
copyloopvar	A linter detects places where loop variables are copied.	✅
cyclop	Checks function and package cyclomatic complexity.
decorder	Check declaration order and count of types, constants, variables and functions.
depguard	Go linter that checks if package imports are in a list of acceptable packages.
dogsled	Checks assignments with too many blank identifiers (e.g. x, , , _, := f()).
dupl	Detects duplicate fragments of code.
dupword	Checks for duplicate words in the source code.
durationcheck	Check for two durations multiplied together.	✅
err113	Go linter to check the errors handling expressions.
errcheck	Check for unchecked errors. Unchecked errors can be critical bugs in some cases.	✅
errchkjson	Checks types passed to the json encoding functions. Reports unsupported types and reports occurrences where the check for the returned error can be omitted.	✅
errname	Checks that sentinel errors are prefixed with the Err and error types are suffixed with the Error.	✅
errorlint	Errorlint is a linter for that can be used to find code that will cause problems with the error wrapping scheme introduced in Go 1.13.	✅
exhaustive	Check exhaustiveness of enum switch statements.
exhaustruct	Checks if all structure fields are initialized.
exptostd	Detects functions from golang.org/x/exp/ that can be replaced by std functions.	✅
fatcontext	Detects nested contexts in loops and function literals.
forbidigo	Forbids identifiers.
forcetypeassert	Finds forced type assertions.
funlen	Checks for long functions.
ginkgolinter	Enforces standards of using ginkgo and gomega.
gocheckcompilerdirectives	Checks that go compiler directive comments (//go:) are valid.
gochecknoglobals	Check that no global variables exist.	✅
gochecknoinits	Checks that no init functions are present in Go code.	✅
gochecksumtype	Run exhaustiveness checks on Go "sum types".
gocognit	Computes and checks the cognitive complexity of functions.
goconst	Finds repeated strings that could be replaced by a constant.
gocritic	Provides diagnostics that check for bugs, performance and style issues.Extensible without recompilation through dynamic rules.Dynamic rules are written declaratively with AST patterns, filters, report message and optional suggestion.	✅
gocyclo	Computes and checks the cyclomatic complexity of functions.
godot	Check if comments end in a period.	✅
godox	Detects usage of FIXME, TODO and other keywords inside comments.
goheader	Checks if file header matches to pattern.
gomoddirectives	Manage the use of 'replace', 'retract', and 'excludes' directives in go.mod.	✅
gomodguard	Allow and block list linter for direct Go module dependencies. This is different from depguard where there are different block types for example version constraints and module recommendations.	✅
goprintffuncname	Checks that printf-like functions are named with f at the end.	✅
gosec	Inspects source code for security problems.	✅
gosmopolitan	Report certain i18n/l10n anti-patterns in your Go codebase.
govet	Report suspicious constructs, such as Printf calls whose arguments do not align with the format string.	✅
grouper	Analyze expression groups.
iface	Detect the incorrect use of interfaces, helping developers avoid interface pollution.
importas	Enforces consistent import aliases.
inamedparam	Reports interfaces with unnamed method parameters.
ineffassign	Detect when assignments to existing variables are not used.	✅
interfacebloat	A linter that checks the number of methods inside an interface.
intrange	Intrange is a linter to find places where for loops could make use of an integer range.
ireturn	Accept Interfaces, Return Concrete Types.
lll	Reports long lines.	✅
loggercheck	Checks key value pairs for common logger libraries (kitlog,klog,logr,zap).
maintidx	Maintidx measures the maintainability index of each function.
makezero	Finds slice declarations with non-zero initial length.	✅
mirror	Reports wrong mirror patterns of bytes/strings usage.
misspell	Finds commonly misspelled English words.	✅
mnd	An analyzer to detect magic numbers.
musttag	Enforce field tags in (un)marshaled structs.
nakedret	Checks that functions with naked returns are not longer than a maximum size (can be zero).	✅
nestif	Reports deeply nested if statements.
nilerr	Finds the code that returns nil even if it checks that the error is not nil.	✅
nilnesserr	Reports constructs that checks for err != nil, but returns a different nil value error.Powered by nilness and nilerr.
nilnil	Checks that there is no simultaneous return of nil error and an invalid value.
nlreturn	Nlreturn checks for a new line before return and branch statements to increase code clarity.
noctx	Finds sending http request without context.Context.	✅
nolintlint	Reports ill-formed or insufficient nolint directives.	✅
nonamedreturns	Reports all named returns.
nosprintfhostport	Checks for misuse of Sprintf to construct a host with port in a URL.
paralleltest	Detects missing usage of t.Parallel() method in your Go test.
perfsprint	Checks that fmt.Sprintf can be replaced with a faster alternative.
prealloc	Finds slice declarations that could potentially be pre-allocated.	✅
predeclared	Find code that shadows one of Go's predeclared identifiers.	✅
promlinter	Check Prometheus metrics naming via promlint.	✅
protogetter	Reports direct reads from proto message fields when getters should be used.	✅
reassign	Checks that package variables are not reassigned.
recvcheck	Checks for receiver type consistency.
revive	Fast, configurable, extensible, flexible, and beautiful linter for Go. Drop-in replacement of golint.	✅
rowserrcheck	Checks whether Rows.Err of rows is checked successfully.	✅
sloglint	Ensure consistent code style when using log/slog.	✅
spancheck	Checks for mistakes with OpenTelemetry/Census spans.	✅
staticcheck	Go vet on steroids, applying a ton of static analysis checks.	✅
sqlclosecheck	Checks that sql.Rows, sql.Stmt, sqlx.NamedStmt, pgx.Query are closed.	✅
tagalign	Check that struct tags are well aligned.
tagliatelle	Checks the struct tags.
testableexamples	Linter checks if examples are testable (have an expected output).
testifylint	Checks usage of github.com/stretchr/testify.
testpackage	Linter that makes you use a separate _test package.
thelper	Thelper detects tests helpers which is not start with t.Helper() method.
tparallel	Tparallel detects inappropriate usage of t.Parallel() method in your Go test codes.	✅
unconvert	Remove unnecessary type conversions.	✅
unused	Check Go code for unused constants, variables, functions and types	✅
unparam	Reports unused function parameters.
usestdlibvars	A linter that detect the possibility to use variables/constants from the Go standard library.	✅
usetesting	Reports uses of functions with replacement inside the testing package.
varnamelen	Checks that the length of a variable's name matches its scope.
wastedassign	Finds wasted assignment statements.	✅
whitespace	Whitespace is a linter that checks for unnecessary newlines at the start and end of functions, if, for, etc.	✅
wrapcheck	Checks that errors returned from external packages are wrapped.
wsl	Add or remove empty lines.
zerologlint	Detects the wrong usage of zerolog that a user forgets to dispatch with Send or Msg.

Notes

• https://golangci-lint.run/product/migration-guide/
• https://golangci-lint.run/usage/formatters/

[fredrikaverpil]

thought: Can we first upgrade golangci to v2 with as few changes as possible, so it behaves as close as today? And then later introduce formatting, so we can have a separate discussion around that?

@thall Yes, I think that's good - I'll remove anything formatting related from the PR.

But just so it is clear, the formatting does not happen on golangci-lint run, so you won't be getting any unexpected formatting all of a sudden.

[thall]

@thall Yes, I think that's good - I'll remove anything formatting related from the PR.

But just so it is clear, the formatting does not happen on golangci-lint run, so you won't be getting any unexpected formatting all of a sudden.

Ah okej, that is good! Lets keep this PR as small as possible then.

[fredrikaverpil]

@thall I left the linters in, which the migration tool added. It's unclear to me why it decided to add those in. I can make a separate PR where those gets added in and we can decide on them separately. Ok?
EDIT: I just realized the "added linters" were just alphabetically sorted. So I think this PR is as constrained in scope as we can get it.

But apart from that, I think primarily we need to get the relative-path-mode right. I set it to gomod now and it seems like it everything works as expected. I can get lint warnings from the "usual suspects folders".

[thall]

@thall I left the linters in, which the migration tool added. It's unclear to me why it decided to add those in. I can make a separate PR where those gets added in and we can decide on them separately. Ok? EDIT: I just realized the "added linters" were just alphabetically sorted. So I think this PR is as constrained in scope as we can get it.

But apart from that, I think primarily we need to get the relative-path-mode right. I set it to gomod now and it seems like it everything works as expected. I can get lint warnings from the "usual suspects folders".

Perfect, looks like it just got sorted. I can try this PR out in some repos as well.

[thall]

why is this removed in the new config?

[fredrikaverpil]

I'm unable to add gci back in:

[go-lint] Error: can't load config: gci is a formatter
[go-lint] Failed executing command with error: can't load config: gci is a formatter

Their migration guide is very confusing around gci/gofumpt. But when looking at the linters page, you can see it is no longer available as linter. I guess we can only use it as a formatter now. See https://golangci-lint.run/usage/formatters/

I'm adding those in here: #662

[thall]

Ah, looks like those are moved under formatters.

[fredrikaverpil]

It was moved out of the linters section and is now a formatter, like with gci.

[go-lint] Error: can't load config: gofumpt is a formatter
[go-lint] Failed executing command with error: can't load config: gofumpt is a formatter

Again, I'm adding it here: #662

[thall]

@fredrikaverpil Tried this out in one of our repos, what is the intended migration path for code like this:

	return sggolangcilint.Run(
		ctx,
		"--exclude-dirs",
		"website",
		"--exclude-dirs",
		"terraform",
		"--exclude-dirs",
		"api",
		"--exclude-dirs",
		"data-mesh",
	)

Fails with:

$ make go-lint
[sage] building binary and generating Makefiles...
[go-lint] linting Go files...
[go-lint] Error: unknown flag: --linters-exclusions-paths
[go-lint] Failed executing command with error: unknown flag: --linters-exclusions-paths
[go-lint] Error: unknown flag: --linters-exclusions-paths
[go-lint] Failed executing command with error: unknown flag: --linters-exclusions-paths
[go-lint] Error: unknown flag: --linters-exclusions-paths
[go-lint] Failed executing command with error: unknown flag: --linters-exclusions-paths
[go-lint] exit status 3

[fredrikaverpil]

what is the intended migration path for code like this

@thall unfortunately, I think exclusions now need to exist in .golangci.yml. They removed the ability to specify them as CLI args completely.

❯ golangci-lint run --help
Run the linters

Usage:
  golangci-lint run [flags]

Flags:
  -c, --config PATH                       Read config from file path PATH
      --no-config                         Don't read config file
      --default string                    Default set of linters to enable (default "standard")
  -D, --disable strings                   Disable specific linter
  -E, --enable strings                    Enable specific linter
      --enable-only strings               Override linters configuration section to only run the specific linter(s)
      --fast-only                         Filter enabled linters to run only fast linters
  -j, --concurrency int                   Number of CPUs to use (Default: Automatically set to match Linux container CPU quota and fall back to the number of logical CPUs in the machine)
      --modules-download-mode string      Modules download mode. If not empty, passed as -mod=<mode> to go tools
      --issues-exit-code int              Exit code when issues were found (default 1)
      --build-tags strings                Build tags
      --timeout duration                  Timeout for total work. Disabled by default
      --tests                             Analyze tests (*_test.go) (default true)
      --allow-parallel-runners            Allow multiple parallel golangci-lint instances running.
                                          If false (default) - golangci-lint acquires file lock on start.
      --allow-serial-runners              Allow multiple golangci-lint instances running, but serialize them around a lock.
                                          If false (default) - golangci-lint exits with an error if it fails to acquire file lock on start.
      --path-prefix string                Path prefix to add to output
      --show-stats                        Show statistics per linter (default true)
      --output.text.path stdout           Output path can be either stdout, `stderr` or path to the file to write to.
      --output.text.print-linter-name     Print linter name in the end of issue text. (default true)
      --output.text.print-issued-lines    Print lines of code with issue. (default true)
      --output.text.colors                Use colors. (default true)
      --output.json.path stdout           Output path can be either stdout, `stderr` or path to the file to write to.
      --output.tab.path stdout            Output path can be either stdout, `stderr` or path to the file to write to.
      --output.tab.print-linter-name      Print linter name in the end of issue text. (default true)
      --output.tab.colors                 Use colors. (default true)
      --output.html.path stdout           Output path can be either stdout, `stderr` or path to the file to write to.
      --output.checkstyle.path stdout     Output path can be either stdout, `stderr` or path to the file to write to.
      --output.code-climate.path stdout   Output path can be either stdout, `stderr` or path to the file to write to.
      --output.junit-xml.path stdout      Output path can be either stdout, `stderr` or path to the file to write to.
      --output.junit-xml.extended         Support extra JUnit XML fields.
      --output.teamcity.path stdout       Output path can be either stdout, `stderr` or path to the file to write to.
      --output.sarif.path stdout          Output path can be either stdout, `stderr` or path to the file to write to.
      --max-issues-per-linter int         Maximum issues count per one linter. Set to 0 to disable (default 50)
      --max-same-issues int               Maximum count of issues with the same text. Set to 0 to disable (default 3)
      --uniq-by-line                      Make issues output unique by line (default true)
  -n, --new                               Show only new issues: if there are unstaged changes or untracked files, only those changes are analyzed, else only changes in HEAD~ are analyzed.
                                          It's a super-useful option for integration of golangci-lint into existing large codebase.
                                          It's not practical to fix all existing issues at the moment of integration: much better to not allow issues in new code.
                                          For CI setups, prefer --new-from-rev=HEAD~, as --new can skip linting the current patch if any scripts generate unstaged files before golangci-lint runs.
      --new-from-rev REV                  Show only new issues created after git revision REV
      --new-from-patch PATH               Show only new issues created in git patch with file path PATH
      --new-from-merge-base string        Show only new issues created after the best common ancestor (merge-base against HEAD)
      --whole-files                       Show issues in any part of update files (requires new-from-rev or new-from-patch)
      --fix                               Fix found issues (if it's supported by the linter)
      --cpu-profile-path string           Path to CPU profile output file
      --mem-profile-path string           Path to memory profile output file
      --print-resources-usage             Print avg and max memory usage of golangci-lint and total time
      --trace-path string                 Path to trace output file

Global Flags:
      --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
  -h, --help           Help for a command
  -v, --verbose        Verbose output

Meaning, you have to remove the --exclude-dirs args from your command.

[fredrikaverpil]

@thall it's worth noting that even if you exclude paths in the config, the directories are still traversed.

From the config file docs:

linters:
  exclusions:

    # Which file paths to exclude: they will be analyzed, but issues from them won't be reported.
    # "/" will be replaced by the current OS file path separator to properly work on Windows.
    # Default: []
    paths:
      - ".*\\.my\\.go$"
      - lib/bad.go
    # Which file paths to not exclude.
    # Default: []
    paths-except:
      - ".*\\.my\\.go$"
      - lib/bad.go

[thall]

@fredrikaverpil I understand why its failing, the problem here is that the config is located within Sage, and thus not possible to modify in order to achieve the same behavior as today.
As I see it, either all consumers needs to have their own config or that it's possible to pass arguments that then are merged with the default config in Sage.
Do you see other alternatives?

[fredrikaverpil]

@thall no, I see it the same way as you do, unless you are fine with skipping those exclusions.

[fredrikaverpil]

@thall not a fantastic name (CommandInDirectoryV2), but this could be used to supply all arguments yourself.

Another alternative is to create other config files for specific use cases next to the golangci.yml we have now.

[kristofferwanglund]

Just a thought, since we have so much Go code and this changes both how the tool itself works and linting results, what do you think about adding a new package sggolangcilintV2 and let everyone migrate at their own pace?

[fredrikaverpil]

@kristofferwanglund yes, I thought about this too, actually. I think the bigger question is how we want to deal with the config file which might no longer be suitable for all projects if different projects needs different exclusion filters.