raising the quality bar: additional linter checks for new code

[Patrick Ohly]

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

[Patrick Ohly]

"Patrick Ohly" <patric...@intel.com> writes:
> I would like propose that we reconsider how we increase code quality in

> Kubernetes. [...] My proposal is that we enable linter checks for

> known issues *before* all old code has been updated.

The basic infrastructure is now in place [1, 2]. The stricter linting
uses the default settings of upstream golangci-lint plus some additional
checks that were already enabled for Kubernetes. Strict linting must be
explicitly requested because the corresponding Prow job does not run
automatically. Let's gather some experience with this before deciding on
how to move forward.

Contributors, please try running "hack/verify-golangci-lint.sh -s -a"
(strict configuration, automatic detection of modified code) while
working on some code change. This ensures that everything still compiles
and helps writing better Go code.

Reviewers, you can start strict linting for a PR with "/test
pull-kubernetes-verify-strict-lint". The result is optional and won't
block merging a PR that has LGTM+approval. It is possible to do this for
a PR that doesn't have an ok-to-test yet. Just doing the linting will be
cheaper than kicking of all test jobs, in particular when further
changes seem to be likely. See [3] for such an example PR.

Results are getting reported as text in the Prow job output. Annotations
for the source code would be visible in the "changed files" view of a PR
and thus easier to handle. Help is needed in [4] to make that happen.

"hack/verify-golangci-lint.sh -s" reports issues in existing code. Let's
*not* start fixing those immediately. Instead, we first need to reach
consensus on which of those are worth fixing and then create issues to
organize that work. Help with this would be welcome.

We don't have to fix everything: it would be okay to be strict only for
new or modified code indefinitely. However, it may be a bit confusing
when developers copy-and-paste existing code or touch something and
suddenly get issues reported.

The next step would be to either run pull-kubernetes-verify-strict-lint
automatically or integrate it into pull-kubernetes-verify. The latter
would be more efficient, but then issues would have to be
merge-blocking: just reporting them without failing the job probably
would just lead to them not being noticed. If we had GitHub annotations,
we could report issues as "for your information" comments without
failing the verification and thus leave it to the developer and
reviewers to decide whether they are worth fixing.

Once some additional linter check passes for the existing code, it can
also get enabled in the normal hack/golangci.yaml. That's not required
because strict linting of PRs will ensure that we don't regress, but
it'll help in situations where someone wants to double-check existing
code or in the periodic ci-kubernetes-verify-master.

[1] https://github.com/kubernetes/kubernetes/pull/109728
[2] https://github.com/kubernetes/test-infra/pull/28914
[3] https://github.com/kubernetes/kubernetes/pull/116325
[4] https://github.com/kubernetes/test-infra/issues/17056

[Antonio Ojea]

This is a great addition Patrick, thanks for working on this

--
You received this message because you are subscribed to the Google Groups "dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to dev+uns...@kubernetes.io.
To view this discussion on the web visit https://groups.google.com/a/kubernetes.io/d/msgid/dev/yrjh7cvr8cje.fsf%40pohly-mobl1.fritz.box.

[Davanum Srinivas]

Indeed, thanks Patrick! We can do an umbrella issue to coordinate the PR(s) and ensure we do meaningful work when 1.28 opens up!

-- Dims

To view this discussion on the web visit https://groups.google.com/a/kubernetes.io/d/msgid/dev/CABhP%3DtbE7ccvhCXYdy8Z2MuwjrtS-7fJgRK-k%2BoyHbLg8H2qRQ%40mail.gmail.com.

--

Davanum Srinivas :: https://twitter.com/dims

[Tim Pepper]

Patrick, thank you thank you thank you!  I’m definitely a fan of consistent (even strict) application of norms in a project because of how it contributes to both long term maintainability and makes new contributors’ experiences easier because there’s less guesswork.

 

With a short look in one area, I like most of the proposed fixes I was given.  I’ll say “gocritic” stands out as opinionated in a few places I maybe disagree with for long term maintainability of code.  What are other seeing?  Any concerns about any of sub-linters?

 

-- 

Tim Pepper

Principal Engineer, Open Source Program Office

VMware, Inc.

 

 

From: Davanum Srinivas <dav...@gmail.com>
Date: Wednesday, March 8, 2023 at 8:03 AM
To: antonio.o...@gmail.com <antonio.o...@gmail.com>
Cc: patric...@intel.com <patric...@intel.com>, d...@kubernetes.io <d...@kubernetes.io>
Subject: Re: raising the quality bar: additional linter checks for new code

Indeed, thanks Patrick! We can do an umbrella issue to coordinate the PR(s) and ensure we do meaningful work when 1.28 opens up!

 

-- Dims

 

On Wed, Mar 8, 2023 at 10:56 AM Antonio Ojea <antonio.o...@gmail.com> wrote:

This is a great addition Patrick, thanks for working on this

 

On Wed, 8 Mar 2023 at 15:54, Patrick Ohly <patric...@intel.com> wrote:

"Patrick Ohly" <patric...@intel.com> writes:
> I would like propose that we reconsider how we increase code quality in
> Kubernetes. [...] My proposal is that we enable linter checks for
> known issues *before* all old code has been updated.

…

[Patrick Ohly]

Tim Pepper <tpe...@vmware.com> writes:

> Patrick, thank you thank you thank you! I’m definitely a fan of consistent (even strict) application of norms in a project because of how it contributes to both long term maintainability and makes new contributors’ experiences easier because there’s less guesswork.
>
> With a short look in one area, I like most of the proposed fixes I was
> given. I’ll say “gocritic” stands out as opinionated in a few places
> I maybe disagree with for long term maintainability of code. What are
> other seeing? Any concerns about any of sub-linters?

For those who couldn't be at the Kubernetes contributor summit today: I
presented the proposal (check for a recording of
https://kcseu2023.sched.com/event/1LXBP/improving-code-quality-enabling-additional-linters)
and also suggested a way how we can gather some opinions about
individual checks.

I have prepared https://github.com/kubernetes/kubernetes/issues/117288
with one comment per issue category. Please use the reaction buttons as
explained in that issue.

My proposal is that we give people one month to try this out and
vote. At that time we may also have a Prow plugin for posting issues as
annotations or at least know how much longer it might take.

If we have it, it'll be easier to roll this out: we could enable all
checks that haven't been rejected outright as just informative, then
later make those that still have strong support merge-blocking.

--
Best Regards

Patrick Ohly
Cloud Software Architect

Intel GmbH
System Software Engineering/Cloud Native

Usenerstr. 5a Phone: +49-228-2493652
53129 Bonn
Germany

[Patrick Ohly]

"Patrick Ohly" <patric...@intel.com> writes:
> My proposal is that we give people one month to try this out and
> vote. At that time we may also have a Prow plugin for posting issues as
> annotations or at least know how much longer it might take.

Discussions around that Prow plugin took longer and it's currently
uncertain whether it'll come.

Plan B: we now have pull-kubernetes-linter-hints running against
each PR. That job is here to stay. It's optional, i.e. won't block
merging even when it reports failures, and you are welcome but not
required to address issues reported in it, unless they also occur in
pull-kubernetes-verify-strict-lint (see below).

Here's an example:
https://github.com/kubernetes/kubernetes/pull/119099 ->
https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/119099/pull-kubernetes-linter-hints/1696510848102567936

That example (using deprecated APIs) is one where developers and
reviewers must decide whether using those APIs is okay. We just
discussed that and the linter config will be updated accordingly (see
https://github.com/kubernetes/kubernetes/pull/120454).

As an interim solution we also have
pull-kubernetes-verify-strict-lint. The intention is that all issues
reported there must be fixed in new or modified code. Currently the job
is still optional while we explore whether it reports the right issues.

If you see something in pull-kubernetes-verify-strict-lint that shouldn't
be mandatory, then please bring it up on #k8s-code-organization or in an
issue.

Once we have enough confidence that the strict checks work as intended,
they will be enabled in pull-kubernetes-verify (making them
merge-blocking!) and pull-kubernetes-verify-strict-lint will be removed.

One benefit of pull-kubernetes-linter-hints is that it completes rather
quickly and thus provides feedback about compile issues sooner than
pull-kubernetes-verify. However, checking before submitting a PR is of
course still better:

hack/verify-golangci-lint.sh -a -n
# -a = automatically detect changes
# -n = enable reporting of nits (= hints)