RFC: switch from pullapprove to native github process

[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.

Similar discussions: opencontainers/runc#2388, opencontainers/runtime-spec#1057

[rhatdan]

SGTM

[thaJeztah]

SGTM 👍

[thaJeztah]

I think it's ok to configure "min number of approvals"; maintainers can still "use the force" if really needed.

[kolyshkin]

@giuseppe WDYT?

[kolyshkin]

copy-paste from @thaJeztah (#137 (comment)):

I also suggest to disable the "squash and merge" and "rebase and merge" options (mostly personal preference, as they clobber the contributor's commits, and with that, their GPG signing)

I agree.

We could try enabling the "auto merge" option that's in beta (merge after required checks complete)

Haven't tried that yet. The only potential downside I see is PR authors should be more careful about marking their PR as draft while they still work on them -- but that's really minor.

[thaJeztah]

Haven't tried that yet. The only potential downside I see is PR authors should be more careful about marking their PR as draft while they still work on them -- but that's really minor.

The auto merge still requires a reviewer to click the "(auto)merge" button, but they can do so before CI completes. GitHub will then merge once all required checks pass.

One possible caveat is pressing the button after CI passed, but before a second reviewer LGTM'd. I think the "second review pending" also qualifies as "required check", so merge would happen directly once the reviewer approved the PR. Technically, that's correct, but could be surprising.

Still, I think it's a nice feature that may be worth a try (although; probably more useful on repositories with a long CI time, saving you the "wait for CI to pass, and come back to click "merge")

[mrunalp]

Sounds good 👍

[kolyshkin]

Implemented. Let me know if there are some rough edges.

[kolyshkin]

I also suggest to disable the "squash and merge" and "rebase and merge" options (mostly personal preference, as they clobber the contributor's commits, and with that, their GPG signing)

Done this, too.