[proposal/rfc] Use github-native review process, say goodbye to pullapprove

[kolyshkin]

This is a proposal to get rid of pullapprove for this repository.

Pullappove was probably added when github did not have its own mechanism to do LGTMs. For quite some time, such a mechanism exists: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/approving-a-pull-request-with-required-reviews

There is also a mechanism to require a certain number of LGTMs before it is possible to merge a PR: https://help.github.com/en/github/administering-a-repository/about-required-reviews-for-pull-requests, although I don't think it is necessary, since all the moderators here are able to count how many green check marks are there in the top left corner of a PR page.

So, unless I am missing something, there is no value that pullapprove adds to what github has.

Please let me know what you think, @opencontainers/runc-maintainers

[AkihiroSuda]

LGTM

[cyphar]

Yeah, sounds reasonable -- not to mention that PullApprove seems to break depending on the phase of the moon. The only question is about self-LGTMs (a maintainer approving their own patch), which I don't know if GitHub has a mechanism to disallow. I wouldn't really mind dropping that rule, though I'd prefer not to drop it purely for technical reasons.

[hqhq]

SGTM

[caniszczyk]

@cyphar I'm fine moving to the GitHub approach and dropping the rule, the community has changed over time and there seems to be a lot more trust now, we could move to the CODEOWNERS approach https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners

[kolyshkin]

the only question is about self-LGTMs (a maintainer approving their own patch), which I don't know if GitHub has a mechanism to disallow

I think we should have a rule of thumb that PR author's own LGTM does not count/matter, and follow this rule, but there is no need to automate it. From the other side, if there is a way to automate it I'm not against this either.

[thaJeztah]

GitHub supports that; can't approve your own PR's

[cyphar]

Oh, in that case I'm 100% on board for this. I'm not sure if we need formal LGTMs for something like this, I could go enable it now.

[kolyshkin]

OK, I am going forward with this.

Note that now the LGTM should not be a mere comment, but rather a selection in the "review changes" dialog (the "review changes" button in the top-right corner while viewing commits).

[cyphar]

It looks like you enabled "Require branches to be up to date before merging" -- this is a change in the merging rules (and means that we will always have to ask users to rebase before we merge). I'm not against having a policy like that, but that's a separate topic to the switch away from PullApprove. So I've disabled it.

[kolyshkin]

@cyphar thanks, I enabled that one by mistake.

[cyphar]

Yeah, I guessed as much. 😉