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 <[email protected]> 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 >
