opencontainers
Consider allowing single-LGTM merge when there is no objection for two weeks
@opencontainers/runc-maintainers
Lots of PR have been stuck after getting a single LGTM. Can we allow single-LGTM merge when there is no objection for two weeks? We should still try to get multiple LGTMs though.
👍🏻 I'm all for it (and it's also a pity that I can't LGTM my own PRs -- since I usually review them as good as others' PRs).
Technically, I guess, this is not possible to implement in github. So maybe "1 LGTM" hard rule (enforced by github), and "2 LGTMs or 1 LGTM + 2 weeks" soft rule (enforced by maintainers).
Doh! Thought I commented, but I must not have finished.
My best answer is "it depends". In general I've always been in a proponent of trust in fellow maintainers; default to 2 LGTMs, but if you (as a maintainer) are sure that a change is safe to take (say, the proverbial "typo fix" in README, some fixes in non-production code (tests, CI)), then I don't think there's a need for some artificial block to prevent merging. The reverse applies as well; I trust fellow maintainers to NOT merge a change even if "2 LGTMs" are present, if they consider a change requires more eyes.
That said; even a 1 line change may have severe consequences in the wrong part of the code, and defaulting to a "single LGTM" is scary. I've seen many cases where 2 reviews saved my behind (first reviewer overlooked an essential bit). So .. it really depends.
I think the problem at the root of this issue is that we're understaffed. I know I (personally) haven't been around as much lately as I wish to be. For some PR's I manage to pick some reviews in between other work, but there's definitely PRs that would require my full attention to do a proper review, and I don't always manage to get to that.
In that respect it's also not just "numbers"; "just add more people" only works to an extent (can help for trivial reviews, but won't help for the "in-depth" knowledge needed in certain areas.
I recently mentioned the kind of things in an internal conversation (different topic, but related) and just looking at the data for the last ~ Year shows that there's a real problem. For a critical component at the root of millions of container systems, these charts are not what you'd want to see; https://github.com/opencontainers/runc/graphs/contributors?from=2021-12-14&to=2023-06-21&type=c
Perhaps we should also look if we can somehow find more backing from projects depending on this code /cc @caniszczyk
This reminds me of discussions we had several years ago about allowing self-LGTMs within OCI (with some OCI projects allowing it and then later disallowing it and vice-versa). I agree with @thaJeztah that ideally whatever number of LGTMs is technically required to merge a PR is not meant to be the be-all and end-all -- maintainers should use their judgement to decide if a change needs more eyes or not. For very simple changes, it seems like overkill to require two maintainers to LGTM something (but then again, having a blanket policy means that we won:t miss things that seem like trivial changes but are actually not).
The elephant in the room is that @kolyshkin has been doing the lions share of the work for the past ~2 years while everyone else has been busy with other work (and for my part at least, sorry about that...) and so the slow review problem is obviously an issue because of this imbalance. There are loads of PRs that I have in my review backlog that I haven't gotten around to, and while I do my best to chip away at it, it feels like a really big mountain of things to review.
On the actual proposal, if we had more active maintainership I would err on the side of rejecting it because I think having a blanket 2-LGTM policy is a good thing, even if most maintainers might do more cursory reviews when the code was written by a fellow maintainer and someone else has already reviewed it. For my part, I will try to rebalance my work schedule to spend more time on runc maintainership, but obviously that alone isn't a solution. Speaking personally, at the moment I have a few projects I'm working on (the magic-link hardening kernel work, libpathrs, some bits of umoci, and in the future the image-spec work) but I anticipate that I should be able to spend more time on runc by the end of the year.
Maybe we can temporarily relax the LGTM requirement to 1-LGTM, but we have a general policy that maintainers should try to get 2 or more LGTMs for most patches, using their own judgement as to whether a patch needs more review. We can then re-evaluate the policy every few months from now to see whether we should keep it (with the hopes that we can get maintainers more involved over time and we can drop the policy once it's no longer necessary).
@kolyshkin
and it's also a pity that I can't LGTM my own PRs -- since I usually review them as good as others' PRs
While the quality of your work is excellent, and you do very detailed reviews, I'm sure you'd agree it's fairly easy to fall into the human biases of overlooking things when you write them yourself. Even for code that I am very sure is obviously correct, I prefer having two maintainers check it so that I can be far more confident I didn't miss anything.
I used to be in the "self-LGTMs should be allowed" camp but I've slowly moved back to thinking that maybe having a blanket 2-LGTM policy is better (but this assumes you have enough active maintainers).
@kolyshkin @thaJeztah @cyphar DMed you on discussing addition of new maintainers.
I guess we can still allow single-LGTM for:
- Documentations
- Tests
- Cherry-picks
FWIW that's what we (informally) do on moby/moby with single-LGTMs, and it works well for us.
