Skip to content

pull-requests.md: add guidance for large and/or automatic edits #8451

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+33 −1
Open

pull-requests.md: add guidance for large and/or automatic edits #8451

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion contributors/guide/pull-requests.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -268,13 +270,18 @@ 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
changes together accordingly (e.g., with one PR touching files in `cmd/kube-proxy` and
`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.
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(parts of this are also redundant with the earlier section "Don't Open Pull Requests That Span the Whole Repository")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I think they are a complimentary, so I cross-referenced them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/kubernetes/community/compare/316bb29bf6c134ed9b0955971f6107ddf4f609b8..f67a105f9a1f379f46d0f7a554dffc3085baa44d

- 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add some suggestions/questions in current Pull Request template for this part?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hesitant about that. The template is pretty long

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be more than happy to drop the "Documentation" section from the template, but that's a different discussion...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree our current template is already long. This is more for contributors to use when they are closing PRs, so they don't have to repeat the same thing over and over again, but rather just use a "saved reply" feature from GH.

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docs guidelines are to use lower case in section titles, but this document is different. I used the opportunity to make this section title consistent and also added it to the TOC.


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
Expand Down