pull-requests.md: add guidance for large and/or automatic edits
This primarily came out of the discussion around allowing the use of LLMs (https://github.com/kubernetes/steering/issues/291), but isn't limited to it because other tools (search/replace, linters) can have the same effect. The goal is to clarify expected behavior and to give reviewers something that they can link to when they decide that a PR shouldn't get merged.
This commit is contained in:
parent
eeaf3e9099
commit
c738487ef5
|
@ -35,6 +35,8 @@ It should serve as a reference for all contributors, and be useful especially to
|
|||
- [It's OK to Push Back](#its-ok-to-push-back)
|
||||
- [Common Sense and Courtesy](#common-sense-and-courtesy)
|
||||
- [Trivial Edits](#trivial-edits)
|
||||
- [Large or Automatic Edits](#large-or-automatic-edits)
|
||||
- [Fixing Linter Issues](#fixing-linter-issues)
|
||||
- [The Testing and Merge Workflow](#the-testing-and-merge-workflow)
|
||||
- [More About `Ok-To-Test`](#more-about-ok-to-test)
|
||||
|
||||
|
@ -268,6 +270,9 @@ handful of people who can approve changes across large portions of the repositor
|
|||
are generally the people who are the most busy and hardest to get reviews from, especially
|
||||
when you're a new contributor with no connections within the community yet.)
|
||||
|
||||
The effort required to review such sweeping changes might not be worth it, see
|
||||
["large or automatic edits")[#large-or-automatic-edits] below.
|
||||
|
||||
If you really want to try to get such a PR merged, your best bet is to break up the PR
|
||||
into separate PRs for each SIG whose code it touches. You can look at the `OWNERS` files
|
||||
in a directory (or its parent directory) to see who owns that code, and then group the
|
||||
|
@ -275,6 +280,8 @@ changes together accordingly (e.g., with one PR touching files in `cmd/kube-prox
|
|||
`pkg/util/iptables`, which are owned by SIG Network, and another PR touching files in
|
||||
`pkg/kubelet` and `pkg/controller/nodelifecycle`, which are owned by SIG Node.)
|
||||
|
||||
|
||||
|
||||
## Comments Matter
|
||||
|
||||
In your code, if someone might not understand why you did something (or you won't remember why later), comment it. Many code-review comments are about this exact issue.
|
||||
|
@ -590,7 +597,32 @@ at once to that file.
|
|||
* Can the file be improved further?
|
||||
* Does the trivial edit greatly improve the quality of the content?
|
||||
|
||||
## Fixing linter issues
|
||||
## Large or Automatic Edits
|
||||
|
||||
Some tools make it very easy to create large Pull Requests, for example:
|
||||
- global search/replace
|
||||
- linters which automatically correct issues (see also next section)
|
||||
- large language models (LLMs) which generate code or documentation
|
||||
|
||||
To make it easier for reviewers to handle such Pull Requests, please explain
|
||||
how it was generated in the "Special notes for your reviewer" section of the
|
||||
Pull Request description. Reviewers may then be able to reproduce those steps
|
||||
(search/replace, linters) or can start the review with the right expectations
|
||||
(LLMs). Also consider the section about [splitting up Pull
|
||||
Requests](#dont-open-pull-requests-that-span-the-whole-repository) above.
|
||||
|
||||
Even with such tools it is still your responsibility as submitter of a Pull
|
||||
Request to ensure that the change is correct (to the best of your knowledge),
|
||||
and that making the change improves the project enough to justify the cost that
|
||||
is needed to review and merge the Pull Request (see previous section). If
|
||||
unsure, create a Draft Pull Request and ask for guidance.
|
||||
|
||||
Please understand that reviewers may decide to close a Pull Request with a
|
||||
reference to this documentation if they come to the conclusion that the
|
||||
difficulty of properly reviewing the Pull Request outweighs the benefit that
|
||||
the Pull Request provides.
|
||||
|
||||
## Fixing Linter Issues
|
||||
|
||||
Kubernetes has a set of linter checks. Some of those must pass in the entire
|
||||
code base, some must pass in new or modified code, and some are merely hints
|
||||
|
|
Loading…
Reference in New Issue