-
Notifications
You must be signed in to change notification settings - Fork 54
Add policy for maintainer takeover of stale PRs #159
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -44,6 +44,31 @@ the feature was conceived and can inform future documentation contributions. | |||||
| Make sure that new tests are added for bugs in order to catch regressions and tests | ||||||
| with new features to exercise the new functionality that is added. | ||||||
|
|
||||||
| ## Maintainer Takeover of Stale Pull Requests | ||||||
|
|
||||||
| The containerd project values the effort that contributors put into pull | ||||||
| requests. However, life priorities change, and sometimes a contributor may | ||||||
|
mikebrow marked this conversation as resolved.
|
||||||
| become unresponsive before a pull request is fully ready to be merged. | ||||||
|
mikebrow marked this conversation as resolved.
|
||||||
|
|
||||||
| If a pull request is nearly complete but requires some final adjustments (such | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some concerns about this wording:
I'm also not sure I agree with the premise of this being to deliver bugs or features faster (see the above comment on "Fast Tracking"). If we want to include this then we probably need another round of review to ensure we're all on the same page.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Commented above, but there's value in both here. It can often be faster for a maintainer to just fix trivial issues in a PR rather than requesting the author does it (which fits the "fast tracking" bit). The unresponsive case is also important.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nod .. value in both... and with my first read it read, to me, like it was for both cases but primarily to un-stick. Wasn't clear in the reading that this was just about reducing the aged PR list to a manageable size. So separating the two tasks seems like a good way to go. |
||||||
| as addressing minor review nits or resolving merge conflicts), and the original | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These examples mirror the style of other sections in this guide and help readers understand what counts as "nearly complete" |
||||||
| author is not responding to comments, a maintainer may choose to take over the | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unresponsive authors were the original premise for this proposal. Including this provides necessary context for why we would choose to take over a PR.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. author may not be able to respond or disagrees with the review.. or the notice just got lost in the mail bin. Best to always be nice to the contributor. Didn't seem to need to be said. I don't want to read, I took over this PR because you (the author) didn't respond fast enough. Prefer the wording we love your diffs and are following it up with ___ to accelerate the merge.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fuweid that is a GitHub permissions feature that allows a maintainer to push to the contributor's remote. I think it's a little independent from the policy though; we can always push to our own remotes and create new PRs in order to carry the work forward. |
||||||
| pull request to ensure the work is not lost and can be merged. | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this line is consistent with the narrative style of this guide and re-iterates that we value the author's contributions |
||||||
|
|
||||||
| In such cases, we follow a practice inspired by the | ||||||
| [Linux kernel contribution guidelines](https://docs.kernel.org/maintainer/modifying-patches.html): | ||||||
|
|
||||||
| - **Authorship Preservation**: The maintainer will preserve the original | ||||||
| author's identity in the commit history (e.g., keeping them as the commit | ||||||
| author or using `Co-authored-by`). | ||||||
| - **Modifications**: The method of applying modifications (e.g., amending | ||||||
| existing commits for minor fixes vs. adding follow-up commits for larger | ||||||
| changes) is left to the maintainer's preference on a case-by-case basis. | ||||||
| - **Audit Trail**: Maintainers should provide a link back to the original pull | ||||||
|
mikebrow marked this conversation as resolved.
|
||||||
| request in the final commit message to maintain traceability. The original | ||||||
| pull request will then be closed with a reference to the new pull request or | ||||||
| commit. | ||||||
|
|
||||||
| ## Commit Messages | ||||||
|
|
||||||
| There are times for one line commit messages and this is not one of them. | ||||||
|
|
||||||

Uh oh!
There was an error while loading. Please reload this page.