I agree with Rob, I don't think any more rules are necessary. John
On Mon, Jun 21, 2021 at 8:34 AM Robert O Butts <[email protected]> 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 <[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 > > > -- John Rushford [email protected]
