Skip to content

Commit aa351ee

Browse files
authored
Merge pull request #8135 from pohly/trivial-prs-linters
PRs: document policy around fixing linter checks
2 parents 24e0159 + e337d98 commit aa351ee

File tree

1 file changed

+52
-0
lines changed

1 file changed

+52
-0
lines changed

contributors/guide/pull-requests.md

Lines changed: 52 additions & 0 deletions
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)