raising the quality bar: additional linter checks for new code

49 views
Skip to first unread message

Patrick Ohly

unread,
May 10, 2022, 9:01:21 AMMay 10
to d...@kubernetes.io
Hello!

I would like propose that we reconsider how we increase code quality in
Kubernetes. We know of several code patterns, in particular in the E2E
tests, that are sub-optimal and that we want to get rid of. For example,
ExpectNoError(err) can cause tests to fail without any useful
information in the failure message when the error is "timed out waiting
for the condition" (https://github.com/kubernetes/kubernetes/issues/109600).

The approach in the past has been to look for volunteers who fix the
existing code and then perhaps at some point in the future enable a
check that all code must pass. What I've seen happening instead is that
some parts of the code base get fixed, then such efforts run out of
steam and new code reintroduces the same issues again.

My proposal is that we enable linter checks for known issues *before*
all old code has been updated. Obviously such checks then can only be
applied to new code in PRs, but that is technically possible. This may
feel unfair to new contributors ("This was fine before, why can't I do
the same in my code?!"), but I think it is a necessary step that the
project needs to take. The original author knows the code best and this
scales better.

The same linter checks can also be applied to the existing code base to
identify places that volunteers could work on. Such efforts should
continue to reduce confusion that comes from existing code not
representing best practices.

Enabling linters for new code can also make the work of maintainers and
reviewers easier: instead of having to manually point out better ways to
write Go code, tools like golangci-lint can do that automatically, more
thoroughly, and often with specific instructions what to write instead.

I've had good experiences with golangci-lint in its default
configuration. Instead of a long debate about what Go code in Kubernetes
should or should not look like, let's trust the decisions made by other
Go developers and use that configuration as a starting point. We can
still tune settings if it turns out to be not suitable for Kubernetes.

The responsibility for those general Go checks probably falls under SIG
Architecture Code Organization. For checks that apply to test/e2e, it
would be SIG Testing. Log call checks would be owned by SIG
Instrumentation, and so on.

I have a proof-of concept in
https://github.com/kubernetes/kubernetes/pull/109728. It currently has a
custom linter for ExpectNoError(err), implemented as a golangci-lint
plugin, but that's just to show that this can be done. The goal is to
start just with the base golangci-lint and add custom linters later.

What is missing is posting of the linter results as GitHub
annotations. Such annotations show up directly in GitHub next to the
source code line, which will be easier to understand thanks to the
additional context. For that part I will need help from SIG Testing
because a normal Prow job lacks permissions to post annotations.

Let's discuss this further here, at the upcoming KubeCon EU, or in the
PR...

--
Best Regards

Patrick Ohly
Cloud Software Architect

Intel GmbH
System Software Engineering/Cloud Native
Reply all
Reply to author
Forward
0 new messages