I will admit, I am often tempted to merge "approved" PRs probably due to my desire to bring down the open PR count (currently 84). And originally, I agreed more with Zach and thought some sort of process/agreement was needed around what "approved" means but after thinking about it more and reading Rob's response maybe the onus is on the merger to make sure it's "ready to merge" (by talking to the contributor and the approver(s)) OR maybe if i'm not involved with a PR (not the contributor or an approver) I should not concern myself with the PR and let contributor/approvers work to get it merged.
TLDR; if you are about to hit the merge button, make sure you talk to the contributor/approvers first (or read the comments like Rob said) to make sure it's ready for merge. On Mon, Jun 21, 2021 at 8:33 AM Robert O Butts <r...@apache.org> wrote: > I'm not really a fan of putting more rules in place around PRs. We already > have a lot of rules around when you're allowed to make a PR, exactly what > it has to look like, exactly what you have to put in the issue text. > > More specifically, it's pretty common to say "I'm approving this one part > of this PR." People can read, I don't think we've ever had a big problem > with PRs getting accidentally merged, because someone "Approved X but not > Y," and people didn't read it and hit merge. > > I also occasionally Approve to mean "I am technically ok with this being > merged in and maybe a Github Issue being created for some missing thing, > but I would really like to see that thing added if the author doesn't > mind." In which case, people should read what I wrote, and not merge it, > unless it's clear the author isn't going to. In which case the merger also > needs to make that Issue, not just Merge without thinking about it. > > In a nutshell, people Merging always need to read the issue, not just look > for "is it approved?" And I think they do, I don't think we have a problem > with that. > > The more rules we have, the harder it is to build community, the harder it > is to get new contributors, the harder it is to keep the contributors we do > have. If we already had a big, thriving community, and had issues with PRs > being unmanageable, it might make more sense. But right now, we struggle > with community building drastically more than with PR management. We need > to be making contributing easier, not harder. > > I understand not having strict rules leads to occasional confusion, and as > you say, accidental merges, and then that has to be managed. But it's > really not that much work to revert, or make a new PR, or whatever the fix > is. It's just git, and the `master` branch isn't release-ready anyway, it's > not a big deal for it to not be perfect at all times. > > With where our project is in terms of community health, I think those minor > inconveniences are drastically outweighed by the cost to community building > of adding more rules around contributing. > > > On Fri, Jun 18, 2021 at 5:02 PM Zach Hoffman <zrhoff...@apache.org> wrote: > > > A number of PRs submitted in the last 6 months were approved without that > > PR being merged. This has caused confusion among committers, and in some > > cases, the PR has been merged before the PR approver intended the PR to > be > > merged. To avoid confusion in the future, it would help if we could agree > > on some guidelines for approving PRs. Here is a draft: > > > > If you approve a PR, you are vouching that: > > 1. You have already tested the PR > > 2. You will not request for the PR to be changed any further before it is > > merged. > > 3. The PR is ready to merge *right away*, and if someone merges it, you > are > > okay with that and accept accountability for your approving PR review. > > > > When not to approve the PR: > > • If you have not tested a PR (especially if you intend to test it before > > merging), do not approve the PR. > > • If you want, or might want, the PR to be changed in some way before it > is > > merged, do not approve the PR. > > > > Cases where you might approve the PR without want it to be immediately > > merged: > > • If you are reviewing only part of a PR and are relying on other > > contributors to review the rest of the PR, approving the PR is okay as > long > > as you make it clear that you are only approving that specific part. For > > example, if a PR affects both Traffic Ops and Traffic Portal and you only > > intend to review the Traffic Portal portion, approving the PR is okay as > > long as you mention that you are only approving the Traffic Portal > portion > > of the PR. > > • If CI (GitHub Actions for us, currently) is running or pending and you > > 100% sure that the CI will pass, approving the PR is okay. For example, > if > > you have already seen that the GitHub Actions pass for that PR, then the > > submitter adds an additional commit that should not keep the GitHub > Actions > > from passing again, approving the PR is okay. > > > > I'm a bit reluctant to submit a PR to add it to our CONTRIBUTING.md ( > > https://github.com/apache/trafficcontrol/blob/master/CONTRIBUTING.md ) > > because the file is already pretty long, and at the bottom it says "Don't > > let all these guidelines discourage you, we're more interested in > community > > involvement than perfection." IMO we should only add things that add > > clarity to our expectations, rather than complicating them. This isn't a > > long-standing issue, and I think the vast majority of our contributors > > already see "approve" as meaning "merge it now". > > > > Thoughts? > > > > -Zach > > >
