Skip to content

Commit e337d98

Browse files
committed
PRs: document policy around linter checks
Occasionally contributors run linters, fix issues found by them (sometimes automatically!) and then create PRs. This creates work for maintainers and is often not useful, so let's write this down. Perhaps future contributors will find it, if not, we can point to it in PRs when closing them.
1 parent cc28c5c commit e337d98

File tree

1 file changed

+52
-0
lines changed

1 file changed

+52
-0
lines changed

contributors/guide/pull-requests.md

+52
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,58 @@ at once to that file.
590590
* Can the file be improved further?
591591
* Does the trivial edit greatly improve the quality of the content?
592592

593+
## Fixing linter issues
594+
595+
Kubernetes has a set of linter checks. Some of those must pass in the entire
596+
code base, some must pass in new or modified code, and some are merely hints
597+
to developers how to improve their code.
598+
599+
Please do not create Pull Requests for issues found by linters without first
600+
reaching out to maintainers on the `#code-organization`
601+
[Slack](http://slack.kubernetes.io) channel to determine whether there is
602+
sufficient interest in fixing such issues.
603+
604+
When it was discussed, make sure to include people who gave the preliminary
605+
approval of this work as well as the link to the discussion on Slack or GitHub
606+
issue into the PR description. This is a good example to follow:
607+
608+
> /area code-organization
609+
>
610+
> This PR fixes linter rules discussed in the Slack https://kubernetes.slack.com/archives/Foo/Bar.
611+
> Preliminary agreement to address those issues were given by @GHHandle1 and @GHHandle2.
612+
>
613+
> /assign @GHHandle1
614+
> /assign @GHHandle2
615+
>
616+
> This PR fixes issues in the package:
617+
> pkg/kubelet
618+
>
619+
> Related PRs for other packages:
620+
> - github.com/link-to-other-PR1
621+
> - github.com/link-to-other-PR2
622+
623+
It does not matter whether the linter is enabled in Kubernetes or not:
624+
- If a linter *is* enabled in
625+
[golangci.yaml](https://github.com/kubernetes/kubernetes/blob/master/hack/golangci.yaml),
626+
then it has already been determined that sweeping changes in the existing
627+
code aren't necessary or just are not worth the cost (e.g. causing rebases of other
628+
Pull Requests or obscuring authorship).
629+
- If a linter *is not* enabled, then it might not be important enough.
630+
- If the check is performed by third party tools which are not integrated in
631+
the Kubernetes CI or proprietary, file a bug or start a discussion about it first.
632+
633+
Such Pull Requests are often large and thus hard to review. When the linter
634+
enforces some opinion or policy, then this is not necessarily something that
635+
applies to Kubernetes. Kubernetes uses the formatting rules enforced by Go.
636+
Stricter rules like specific usage of
637+
[whitespace](https://golangci-lint.run/usage/linters/#whitespace) or using
638+
[standard library constants](https://golangci-lint.run/usage/linters/#usestdlibvars)
639+
are opinionated and not worth the cost of introducing them now.
640+
641+
Linters worth considering are those which actually improve code correctness,
642+
for example by warning about suspicious code like calling a function and then
643+
not checking the error result.
644+
593645
# The Testing and Merge Workflow
594646

595647
The Kubernetes merge workflow uses labels, applied by [commands](https://prow.k8s.io/command-help) via comments.

0 commit comments

Comments
 (0)