Change PR review procedure for liboqs?

[baentsch]

Currently, PRs to liboqs require approving reviews by 2 people to land. This was introduced at a time when contributors and maintainers were more numerous and between them had more time to diligently review PRs. This arguably is not the case any more (too few people with too many other things at their hands).

This is therefore to discuss removing the need for a second review as
a) superficial reviews by 2 people each with insufficient time at their hands may yield worse overall code than 1 diligent review per PR and
b) OQS is meant to support the research community which may be more forgiving to errors and prefer speed of features over perfection and
c) there is quite a bit of (more) CI (than before the 2-approval rule was introduced) meant to catch quite a few possible PR problems

[dstebila]

An interesting suggestion. I think we are starting to run into delays from lack of code reviews. Some relatively straightforward PRs to fix CI failures (#2258, #2261, #2266) have been waiting longer than I would like. You are right that we do have much more extensive CI than we did a few years ago. I think ideally we'd still get 2 reviews on code, but if we are having trouble getting enough reviews to get code landed, then I think we could move forward with 1.

Perhaps we could leave the configuration as is (requiring 2 reviews), but give the maintainers permission to choose to override and merge with only 1 review based on their judgement. Factors contributing to proceeding with only 1 review might include: size and scope of the PR; whether the PR is coming from a known contributor; whether it touches security sensitive code or not.

[dstebila]

For example, I'm quite tempted to move forward on #2266 with just 1 review to get CI running green again so we can unblock other work.

[baentsch]

Sounds good to me. I couldn't (read: didn't want to) provide a review because I currently don't feel sufficiently competent regarding the code paths in the PR.

[baentsch]

Sounds like a good approach @dstebila; to retain some of the assumed benefits of 4 eyes reviews (more quality reviews&code), what about adding the proviso/understanding that if such "2 eyes only" review introduces problems to main branch (e.g., breaking CI here or downstream), the maintainer accepts responsibility to fix this with highest priority himself? Goal of this would be that said approver is motivated to look carefully (and fix if necessary) irrespective of other tasks/time pressures, thus also taking a bit of the pressure to be responsible for a perfectly fine PR off the PR author's shoulders (and require approvers, maintainer or otherwise, to be/remain well-versed in the system, even if not otherwise actively contributing).

What could also be done is the introduction of an "urgent" tag on a PR such as to make the rest of the team aware of a blocker for which speedy attention/review would be goodness. If this also doesn't help to get more reviews more quickly, I'd suggest dropping the 4 eyes review requirement completely, with the above added approver responsibility documented (again, to get not more, but more thorough (or "responsibility shared"), reviews).