I just want to re-enforce that there are no guarantees that the master branch is production quality code. We do our best to ensure it is, but it is not a release branch which is production-ready code. I say that to say that if a PR is approved and merged before it is ready, we can just make a new PR to finish up whatever was missing. It's not that big of a deal really. Less process is better.
On Mon, Jun 21, 2021 at 9:09 AM Rawlin Peters <raw...@apache.org> wrote: > I also think we shouldn't be putting more rules in place, but I'm glad > you brought this up, Zach, because we definitely want to merge more > PRs (which is a good thing -- we already have too many open PRs). I > think this is a good reminder for committers to know that they're > entrusted with the overall success of the project and that we can all > do better to communicate our intentions with respect to PR approvals. > > - Rawlin > > On Mon, Jun 21, 2021 at 8:55 AM Jeremy Mitchell <mitchell...@gmail.com> > wrote: > > > > 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 > > > > > > > >
