Require approval before merging?

[jeremyevans]

@perlun What are your thoughts on requiring approval before merging? Currently, this is required for rack/rack-test (not sure if it was required before the move to the rack organization). This means maintainers must submit pull requests and have them approved by another maintainer, they cannot push any change, no matter how small, directly to the master branch. @ioquatix and I would like your input before possibly changing the repository configuration.

[ioquatix]

Just for context, I can't recall whether I set it up when we originally migrated the repo, however at least recently when I checked, approval was required by default - I can't recall that I added that - but it's possible that I did it when I moved the repo - it's also equally possible that's just how it was set up.

I'd personally like to see us have a consistent and standard model for PR approval, and I know this is a pain for small PRs, but this gem has got 350m downloads so I think the bar should be set appropriately.

[perlun]

Sorry for the late reply on my part @jeremyevans; I am bad at managing my GitHub inbox at the moment. @ioquatix has a quite valid point; 350M downloads is a significant amount of (potential) users. In an ideal world, requiring PR approval is "the way to go" for this kind of project/repo.

Having that said, I regularly work in an open source project (https://github.com/perlang-org/perlang for the record) where 0% of the PRs ever get peer reviewed, simply because there are no other project contributors. 🙂 So it's obviously a bit of a balance here.

(If I must choose between "validated by CI" and "approved by another human", I would choose the former over the latter (i.e. enforcing PRs to build cleanly). Test/spec failures should be taken seriously and not ignored, and a good way to ensure this is to enforce a PR -based workflow.)

So perhaps something like "PRs strongly preferred, but not strictly enforced" could be one way to tackle this? 🤔 I.e. use it whenever there's a slight chance of concern, but feel free to push a simple typo-fix in a README file straight to master (or main since that's what this repo uses nowadays).