docs: add page on pr merge governance #951
Conversation
✅ Deploy Preview for all-contributors ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
JimMadge
force-pushed
the
maintainers
branch
2 times, most recently
from
6d56c50 to
584e469
Compare
JimMadge
left a comment
JimMadge
left a comment
There was a problem hiding this comment.
I've fixed some linting problems.
It would be nice to move the hints to Starlight asides.
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
Looks great to me, very sensible. Thanks! 🙌
src/content/docs/project/maintain.md
Outdated
| * **All-contributors bot PRs**: Any maintainer can merge without approval | ||
| * **Dependabot PRs**: Any maintainer can merge once CI is green | ||
|
|
||
| ### One approval required |
There was a problem hiding this comment.
Suggestion: can we make a policy of leaving approval-required PRs open for a few business days to give maintainers a chance to look at them? I've been in the awkward situation before where a PR gets merged before I have a chance to look at it and provide the feedback I wanted to.
Example prior art: https://typescript-eslint.io/maintenance/pull-requests#approvals
There was a problem hiding this comment.
Ahhh thanks for that example @JoshuaKGoldberg I'm open to that. I think the downside is that when we are busy, PRs can sit for longer periods. For instance, during periods of heavy development, when we're creating small, iterative PRs, it's really nice to have a cadence where we are merging frequently. That way, if people go offline for some time, the work doesn't sit or we don't forget about a PR.
On the other hand, I see the potential benefit to waiting.
What about a middle ground?
If it's a security fix, a bug fix, or a typo fix that is small in nature, we can merge with one approval. SO, for instance, I might submit a pr to move an MD file to MDX, and the only content changes are small typos and broken links. I think we could just merge.
If we are adding new content - such as this pr, which proposes a new process- we ping who we want to look at it (the maintainer team), get 1-2 approvals depending on the nature of the change. Wait 3 days from the last comment that was not submitted by the pr author, and then merge.
I am a big fan of fast iterative pr's that are small, easy to review and can keep the project moving forward.
Does that sound reasonable? @JimMadge thoughts?
There was a problem hiding this comment.
That sounds reasonable to me 🙂
There was a problem hiding this comment.
I've definitely felt the negatives of both approaches 😆. Waiting too long to merge things and ending up with conflicts (or never merging), but also merging too fast and having to open issues to fix something that could have been caught in review.
In general, I think it is better to merge quickly.
What tools does GitHub give us to manage this? We can increase the number of required reviews. I don't know if there is a way to add a minimum time.
Being more careful with PRs that make significant changes also seems sensible.
There was a problem hiding this comment.
Hi friends. I've been traveling for work and am slowly getting caught up. After considering this further, I am convinced that we should maintain the single review workflow we have been using for the past few months. We don't have the tools to keep track of the 3-day period that I suggested, and our team is thin right now!
If reviews slow down, I know that my work here will also slow down a lot! And, we have a lot to do.
So I am going to make a few more changes to this PR, and then let's try to get it merged so that at least we are all empowered to merge things moving forward!!
AND to address teh concern of other maintainers being interested in providing feedback, I think the best practice there should be - if another maintainer sees something that they'd like to see fixed in the PR, open a new issue! That will keep us moving forward while also allowing others to contribute and build up top of each other's work!! 🚀
There was a problem hiding this comment.
That sounds good to me. Request changes is valid, but especially when the PR works or the change is more tangential I really like the model of identify the problem and open an issue.
There was a problem hiding this comment.
Ok fantastic - i'm fixing linting. Let's see how things go. If we aren't happy with the process, we can change it - no problem.
|
Ok fantastic. Im at a meeting all this week but I'll move to mdx w proper callouts in a new commit |
|
ok friends - time for another review 🚀 |
Co-authored-by: Jim Madge <jim+github@jmadge.com>
Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 6.3.5 to 6.3.6. - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/v6.3.6/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v6.3.6/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-version: 6.3.6 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Leah Wasser <leah@pyopensci.org> Co-authored-by: Jim Madge <jim@jmadge.com>
JimMadge
left a comment
JimMadge
left a comment
There was a problem hiding this comment.
Looks good. I like the spirit of this.
3 participants
closes #958
Based on our last call, we talked about documenting the PR merge workflow. So this pr suggests a new page that provides guidelines for merging prs.
What:
Documentation updates
Why:
NA
How: