pull-requests.md: add guidance for large and/or automatic edits #8451
Conversation
|
@pohly: GitHub didn't allow me to request PR reviews from the following users: kubernetes/steering-committee. Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
cncf-cla: yes
| reference to this documentation if they come to the conclusion that the Pull | ||
| Request is not worth the effort. | ||
|
|
||
| ## Fixing Linter Issues |
There was a problem hiding this comment.
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.
|
|
||
| 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) |
There was a problem hiding this comment.
(parts of this are also redundant with the earlier section "Don't Open Pull Requests That Span the Whole Repository")
There was a problem hiding this comment.
True. I think they are a complimentary, so I cross-referenced them.
There was a problem hiding this comment.
contributors/guide/pull-requests.md
Outdated
| 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 Pull | ||
| Request is not worth the effort. |
There was a problem hiding this comment.
maybe clarify what that means a little more?
| 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 Pull | |
| Request is not worth the effort. | |
| 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 PR provides. |
pohly
force-pushed
the
pr-guidelines-large-automatic-edits
branch
from
316bb29 to
f67a105
Compare
k8s-ci-robot
added
size/M
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
| - 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 |
There was a problem hiding this comment.
Should we add some suggestions/questions in current Pull Request template for this part?
There was a problem hiding this comment.
Hesitant about that. The template is pretty long
There was a problem hiding this comment.
I would be more than happy to drop the "Documentation" section from the template, but that's a different discussion...
There was a problem hiding this comment.
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.
|
/approve |
contributors/guide/pull-requests.md
Outdated
| 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, add a "RFC: " (request for comments) or "WIP: " (work in progress) prefix |
There was a problem hiding this comment.
WIP: has meaning to our CI in a way RFC: does not
There was a problem hiding this comment.
They are both not mentioned elsewhere in the document. I was wondering whether they deserve a more complete description and then decided against it because this didn't look like the right place for it. But perhaps a few more sentence are fine?
RFC is merely informational. WIP also prevents merging accidentally. To avoid running tests before the Pull Request is ready, create a Draft Pull Request, in which case
/testcan be used to trigger only specific test jobs.
There was a problem hiding this comment.
Maybe instead of RFC, just call it a draft PR, which is a GH feature that is even better than WIP, b/c it doesn't run any tests automatically, but rather relies on PR author to explicitly request them.
There was a problem hiding this comment.
Agreed, mentioning one option here that has the desired effect is better than trying to cover all of them. Changed to:
If unsure, create a draft Pull Request and ask for guidance.
There was a problem hiding this comment.
I was unsure whether I should use "Draft Pull Request" or "draft Pull Request". The document uses "Pull Request", but it wasn't entirely clear to me why capital letters where chosen - I just did the same to be consistent. But "draft" is not used elsewhere, so I decided to use the normal spelling.
There was a problem hiding this comment.
Let's have a debate over a single character difference 😝 ...
There was a problem hiding this comment.
GH UI has "Draft Pull Request" as the button you'll see normally, instead of "Create Pull Request" so let's go with that 😉
There was a problem hiding this comment.
That's styling in the web UI. In normal prose, GitHub doesn't capitalize: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/about-pull-request-reviews
But I agree, we probably used "Pull Request" because of the button, so let's stay consistent. Changed...
pohly
force-pushed
the
pr-guidelines-large-automatic-edits
branch
from
f67a105 to
3cb90ee
Compare
k8s-ci-robot
removed
the
lgtm
contributors/guide/pull-requests.md
Outdated
| 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. |
There was a problem hiding this comment.
| unsure, create a draft Pull Request and ask for guidance. | |
| unsure, create a Draft Pull Request and ask for guidance. |
as mentioned elsewhere, this will be consistent with github UI, ie. we won't have to explain it 😉
This primarily came out of the discussion around allowing the use of LLMs (kubernetes/steering#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.
pohly
force-pushed
the
pr-guidelines-large-automatic-edits
branch
from
3cb90ee to
c738487
Compare
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacoxu, pohly, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This primarily came out of the discussion around allowing the use of LLMs (kubernetes/steering#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.
Which issue(s) this PR fixes:
Fixes kubernetes/steering#291
/cc @kubernetes/steering-committee @danwinship