https://git.k8s.io/enhancements/keps/sig-testing/5468-invariant-testing
introduced a mechanism for tests which hook into the test suite run via
ginkgo.ReportAfterSuite. Usage was limited to code in test/e2e/invariants with
stricter reviews.
However, this was not enforced mechanically. With forbidigo we can be sure that
nothing slips through.
ginkgo.ReportBeforeSuite has similar restrictions.
This eliminates the last two remaining three-step value swaps (t =
a; a = b; b = t instead of a, b = b, a) and enables linting to prevent
new ones being added in future.
Signed-off-by: Stephen Kitt <skitt@redhat.com>
This replaces functions that wrap another function with no change with
a direct reference to the wrapped function.
Signed-off-by: Stephen Kitt <skitt@redhat.com>
The golangci-lint docs indicate that, for gocritic, disabled-checks
and enabled-checks can't be combined. That is however not accurate;
enabled-checks adds checks, disabled-checks disables them, in that
order. If disable-all isn't set, then the setup starts with the checks
enabled by default, adds any additional checks specified in
enabled-checks, and removes checks specified in disabled-checks.
This means that instead of filtering unwanted checks, we can disable
them entirely. This makes the set of disabled checks easier to read,
and perhaps easier to clean up in future.
For singleCaseSwitch and wrapperFunc, we only want to ignore specific
instances, so this is still done by filtering.
Signed-off-by: Stephen Kitt <skitt@redhat.com>
Disable QF1008 in pull-kubernetes-linter-hints, based on discussion in
\#k8s-code-organization. We might want a check that forced a consistent
style within a single file, but there is no reason to prefer a global
choice between `obj.ObjectMeta.Name` and `obj.Name`.
Change-Id: If2e7a3a464cdf1ac83fdaaa18b065df1f19e8568
This started to show up now as linter hints at the start of the 1.34 cycle in
all PRs which modify the API. We don't want to enforce that convention in that
generated code, so suppressing it.
kubeadm has replaced pkg/errors with an internal implementation, which
means that pkg/errors can be marked as unwanted (once a final
straggler is replaced).
Signed-off-by: Stephen Kitt <skitt@redhat.com>
Now that all other uses have been removed, this configures depguard to
ensure that new ones aren't added outside kubeadm.
Signed-off-by: Stephen Kitt <skitt@redhat.com>
The linter suppression got removed in the migration to golangci-lint v2 because
it wasn't triggered for existing code, but as seen in a PR which adds a
conversion package the suppression is still needed.
Furthermore, we also want to suppress for unexported, manually written
conversion and defaulting functions because those follow the same Kubernetes
naming convention.
This finishes the work started in
https://github.com/kubernetes/kubernetes/pull/131113.
Changes:
- Remove TODOs for things that we don't have plans to fix.
- Add issue for older TODO.
- Reorganize and remove suppression rules so that the base check
has no unused rules.
- Document warn-unused, but don't enable it.
- Remove disabling of statistics (they are useful) and ensure that
they don't get the ERROR prefix.
- Avoid ../ prefix in paths via `run.relative-path-mode: gomod`.
These suppressions are necessary to make golangci-lint 1.64 pass with the
current code base. This change is meant to be backported to release
branches. On master, we may want to revert some of it together with fixing the
findings.
The corresponding "pull-kubernetes-verify-lint" job was already removed
earlier. Manual strict checking was still possible, but doesn't really make
sense for the same reasons why the job was removed (e.g. the decisions which
checks should be "strict" were too arbitrary).
The explanations for "hints" no longer end with "In general please prefer to
fix the error, ..." because that was misleading and only really applied to the
checks for existing code. For those checks we prefer to fix errors instead of
suppressing them, but not for hints.
This is a workaround for a false positive in the staticcheck linter,
which has not been addressed yet.
See https://github.com/dominikh/go-tools/issues/1294.
Once this is fixed in staticcheck, we can just remove the exclude rule
from the golangci config.
Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
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.
