Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kolyshkin opened this issue Jul 22, 2020 · 13 comments
Closed

Comments

@kolyshkin
Copy link
Contributor

(similar to opencontainers/runc#2388)

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/runtime-spec-maintainers

@vbatts
Copy link
Member

vbatts commented Jul 22, 2020

yes, please

@crosbymichael
Copy link
Member

LGTM

1 similar comment
@hqhq
Copy link
Contributor

hqhq commented Jul 23, 2020

LGTM

@thaJeztah
Copy link
Member

LGTM (non binding)

@kolyshkin
Copy link
Contributor Author

@tianon @mrunalp @caniszczyk PTAL

@tianon
Copy link
Member

tianon commented Aug 19, 2020

LGTM, although I think it's useful to have it require the proper number (requires less cognitive effort to verify, so more likely to get it right consistently)

@mrunalp
Copy link
Contributor

mrunalp commented Aug 19, 2020

LGTM

@thaJeztah
Copy link
Member

although I think it's useful to have it require the proper number

Yes, it's possible to configure the number of approvals required (and to disallow "stale" reviews)

@crosbymichael
Copy link
Member

LGTM

@kolyshkin
Copy link
Contributor Author

Looks like we have a consensus, and so we can move on to implementing the change. I do not have any admin rights over that repo, so can anyone from @opencontainers/runtime-spec-maintainers please implement that?

@cyphar
Copy link
Member

cyphar commented Feb 6, 2021

@caniszczyk can set this up.

@kolyshkin
Copy link
Contributor Author

Alas, this is still not implemented, nor do I have admin access to do it. @caniszczyk please? 🙏🏻

@AkihiroSuda
Copy link
Member

Seems completed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants