Prior to this it would download Go, setup the environment, then (maybe)
download Go again.
I renamed verify_go_version to be internal and fixed all callers.
The in-tree configs use a relative path to find logcheck.so. This is useful
because then the invocation of golangci-lint also works outside of the script.
But when running with a containerized build, GOBIN points somewhere else. For
that case, a temporary copy of the configuration has to be created with an
absolute path.
Contributors have created PRs because the linter found them. That is often not
necessary and should be discussed before working on a PR.
While at it, output formatting gets updated a bit (extra blank lines, wording).
The voting in https://github.com/kubernetes/kubernetes/issues/117288 led to
one check that got rejected ("ifElseChain: rewrite if-else to switch
statement") and several that are "nice to know".
golangci-lint's support for issue "severity" is too limited to identify "nice
to know" issues in the output (filtering is only by linter without considering
the issue text; not part of text output). Therefore a third configuration gets
added which emits all issues (must fix and nits). The intention is to use
the "strict" configuration in pull-kubernetes-verify and the "hints"
configuration in a new non-blocking pull-kubernetes-linter-hints.
That way, "must fix" issues will block merging while issues that may be useful
will show up in a failed optional job. However, that job then also contains
"must fix" issues, partly because filtering out those would make the
configuration a lot larger and is likely to be unreliably (all "must fix"
issues would need to be identified and listed), partly because it may be useful
to have all issues in one place.
The previous approach of manually keeping two configs in sync with special
comments didn't scale to three configs. Now a single golangci.yaml.in with
text/template constructs contains the source for all three configs. A new
simple CLI frontend for text/template (cmd/gotemplate) is used by
hack/update-golangci-lint-config.sh to generate the three flavors.
That release is the first one with official support for Go 1.21. go-ruleguard
must be >= 0.3.20 because of
https://github.com/quasilyte/go-ruleguard/issues/449 with Go
1.21. golangci-lint itself doesn't depend on a recent enough release yet, so
this was done manually.
When sh2ju.sh was called to generate the junit_verify.xml, it used to include
the entire output of a failed script twice: once as failure message, once as
log output.
This output can be large and often the actual failure isn't near the top, but
rather at the end or (in the case of the different golangci-lint invocations)
embedded in the log. This makes them hard to see at a glance when looking at
the Prow result page for a job.
Now a verify script can prefix relevant lines with "ERROR: " and then only
those lines are used as failure message in JUnit, without that prefix.
That string was chosen because Prow itself also then picks up those lines when
viewing the entire build log and it is unlikely that some script prints such
lines when they are not meant to be part of the failure.
If some script outputs no such lines, "see stderr for details" is used as
failure message. This is better than before because it avoids the redundancy.
The long-term goal is that when "make verify" is invoked in pull job, it will
also run golangci-lint with the strict configuration and write an
$ARTIFACTS/golangci-lint-githubactions.log file with GitHub actions
annotations. How to get those published for the GitHub PR is open.
When "make verify" is invoked manually or in any other job, the stricter check
will be skipped. That works because "PR_NUMBER" is only set for pre-merge
jobs (https://github.com/kubernetes/test-infra/blob/master/prow/jobs.md#job-environment-variables).
Because strict linting is still new and might not be useful without a better
UI (= GitHub annotations), this PR does not fully enable the integration into
"make verify". Instead, the new verify-golangci-lint-pr.sh is excluded from it
and needs to be run in a separate job.
It is useful to check new code with a stricter configuration because we want it
to be of higher quality. Code reviews also become easier when reviewers don't
need to point out inefficient code manually.
What exactly should be enabled is up for debate. The current config uses the
golangci-lint defaults plus everything that is enabled explicitly by the normal
golangci.yaml, just to be on the safe side.
To run just a specific linter based on command line flags, the default
configuration needs to be disabled (because it would enable additional ones)
and then command line flags must be passed through to "golangci-lint run".
For example, to lint with just "go vet" in verbose mode, use:
verify-golangci-lint.sh -c none -- --disable-all --enable=govet -v
Because the script now explicitly selects the configuration file, the files no
longer have to be in the root directory. Having them in hack without the
leading dot is better because they then have the same owners as the script and
are more visible.
The downside is that manual invocations of golangci-lint without the parameter
no longer work.
Running logcheck as part of golangci-lint has several advantages:
- faster checking because finding files and parsing is shared
with other linters
- gets rid of the complex and buggy
hack/verify-structured-logging.sh (https://github.com/kubernetes/kubernetes/issues/106746)
- support for // nolint:logcheck
- works with Go 1.18
