Set minimum PR approvals to 1 or 2?

[ioquatix]

I would like to propose that we set the minimum PR approvals to 1 or 2.

Right now, it would be hard to get 2 approvals because it's mostly just myself and @jeremyevans actively working on the code.

However, if we can get another maintainer to actively help, we could consider setting this to 2.

We could also introduce CODEOWNERS file to allow people who care about specific things to be pulled into discussions/issues/reviews more pro-actively.

@rafaelfranca @tenderlove do you have any advice or suggestions about how we can ensure PRs are reviewed by active maintainers before being merged?

[jeremyevans]

I'm not sure that we need an enforced minimum number of approvals. It depends on the PR. Smaller PRs (doc fixes, obvious bug fixes) are fine with one approval, but I agree that more significant PRs would benefit from multiple approvals.

They way it's been going the last couple years, if @ioquatix and I agree on something, it usually gets merged. It's only when we disagree that we have to have another maintainer (usually @tenderlove) break the tie. I understand that all maintainers are busy, and some are busier than others. Nobody is paid to work on Rack, so I'm thankful for any time maintainers can spare.

We're almost ready to ship an alpha/beta version Rack 3, the only issues remaining are whether/how to standardize rack.after_reply (#1802) and rack.early_hints (#1831). If other maintainers can review those and provide feedback, that would be great!

[ioquatix]

Oh, I literally thought you said: "fine with one approval"

[jeremyevans]

If you want to cherry-pick part of my comment, how about the first part: "I'm not sure that we need an enforced minimum number of approvals.". Again, I'm requesting you revert this change until it is approved by a majority of the current maintainers.

[ioquatix]

Okay done.

[tenderlove]

I'm not sure that we need an enforced minimum number of approvals. It depends on the PR. Smaller PRs (doc fixes, obvious bug fixes) are fine with one approval, but I agree that more significant PRs would benefit from multiple approvals.

I think setting a minimum of one approval is fine. If we find the requirement to be too onerous, we can revert it.

[ioquatix]

Okay, done!

[simi]

I don't see any problem with merging stuff before release. It can be reverted at any time if needed. But it is good idea to synchronize before release. Also considering there are 4 maintainers today (if I understand it well), and all of them are really busy, it would be nice to not block smaller PRs as @jeremyevans mentioned by waiting for secondary review.

[ioquatix]

I don't see any problem with merging stuff before release. It can be reverted at any time if needed.

I don't like this model of development because it moves the onus of fixing things onto someone else (like a maintainer) rather than the original developer. We shouldn't be merging code that isn't reviewed thoroughly into Rack. The blast radius is simply too large to get wrong. I'd personally rather move slowly and deliberately.

[simi]

In my eyes you're just trying to solve virtual problem here, that's all. It seems to be based on merge of #1839 (comment) which happens within few minutes (since I have asked in-directly Rafael to review PR for 2.2 series (#1838) and I have added CI for 2.2 branch to make it simpler to cut a new release). As you mentioned no harm was done and any working CI is better than nothing. I'm totally missing what kind of problem are you trying to solve here.

[ioquatix]

I'm totally missing what kind of problem are you trying to solve here.

When I saw that PR initially I didn't know anything about it. All I saw was a PR that was proposed and merged within 8 minutes with little to no discussion. It's not the contents of the PR that bothers me it's the process and lack of communication with key stake holders.

[ioquatix]

Let me add, that on reviewing both merged PRs, I identified issues that should have probably been fixed in the PR before merging and both PRs might need follow up work. I've added comments which I hope are helpful.