Wrapping errors may or may not be the right thing to do (see
https://go.dev/blog/go1.13-errors#whether-to-wrap and the discussion in
https://github.com/kubernetes/kubernetes/issues/123234). But developers should
at least think about it, so let's emit linter hints for it: the golangci-lint
config by default enables it for go-errorlint, just not the linter itself, so
we just need to add it for the "hints" config.
Direct error comparisons and assertions also get checked. Those are typically
something that should be replaced by errors.Is and errors.As, but as the
existing code often doesn't do that, let's also treat those as just hints.
`BeTrue` without explanation in `Should` just prints "false is not true", which
isn't a useful failure message. Even with an explanation that message is still
printed, which is just noise. The new BeTrue/FalseBecause avoid that by
requiring that an explanation is provided and using that instead of "false is
not true".
In Kubernetes, we can enforce the usage of those better alternatives or the
if+Fail combination via forbidigo. Because we have existing code which still
needs to be updated, this can only be a hint for now.
Some examples found with this:
ERROR: test/e2e/apimachinery/protocol.go:75:20: use of `o.BeTrue` forbidden because "it does not produce a good failure message, use BeTrueBecause with an explicit printf-style failure message instead or plain Go: if ... { ginkgo.Fail(...) }" (forbidigo)
ERROR: o.Expect(ok).To(o.BeTrue())
ERROR: ^
ERROR: test/e2e_node/unknown_pods_test.go:163:52: use of `gomega.BeTrue` forbidden because "it does not produce a good failure message, use BeTrueBecause with an explicit printf-style failure message instead or plain Go: if ... { ginkgo.Fail(...) }" (forbidigo)
ERROR: }, f.Timeouts.PodStart, f.Timeouts.Poll).Should(gomega.BeTrue())
ERROR: ^
The solution might also be to write a custom matcher, but that is a bit hard to
explain in the forbidigo message.
39df946c06 was meant to enable stricter comment checking only for cmd/kubeadm
because the maintainers of that want that. However, the exclude rule didn't
capture all possible error texts and therefore some checks were enabled across
the entire code base.
The extended pattern is now based on the golangci-lint source code.
Also, the hint config didn't suppress any of these checks.
Some code owners might want this for specific packages, like cmd/kubeadm.
This cannot be enabled for everything because:
- a lot of existing code doesn't pass (-> can't be in base config)
- a lot of packages don't need it (-> shouldn't even be a hint)
This particualar warning didn't make it into
https://github.com/kubernetes/kubernetes/issues/117288. Discussion on Slack
concluded that "it's hard to have a universal policy for all functions marked
deprecated" and thus this can only be a hint which must be considered on a
case-by-case basis.
For example, APIs like sets.String are very unlikely to ever go away, therefore
it is entirely up to developers whether they switch to sets.Set even though
sets.String is marked as deprecated.
Ideally, the deprecation message should explain this. It doesn't for sets ("use
generic Set instead"), so a better message in that case would have been
"consider using generic Set instead".
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.
