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
