[llvm] [GitHub] Add workflows to manage merging of PRs from authors without commit access (PR #124910)
Louis Dionne via llvm-commits
llvm-commits at lists.llvm.org
Thu Feb 20 06:53:58 PST 2025
ldionne wrote:
> libcxx [documents](https://libcxx.llvm.org/Contributing.html#the-review-process) that you should have 2 approvals from the reviewers group, but does not seem to use any GitHub feature to enforce that. Is that the case @ldionne ?
Our documentation is a bit outdated, this was in the Phabricator times. We haven't revisited that, but the current de facto process is that people are free to merge when the `reviewers-libcxx` group has approved (which requires 1 approval from someone in the group).
General comment about the workflow: It seems surprising to me that we're using a label named `no-commit-access` to power this workflow since we remove the label after the PR has been pinged once. It seems to me that such a label describes whether the PR creator has commit access, and that doesn't change because this workflow as run. It might be worth renaming the label to something else.
>From my perspective, what would be the most helpful (to libc++ at least) is a workflow that checks PRs that have been open for longer than "some amount of time" and which have been approved, and pings them. In that comment, you can also mention that the reviewers should be the ones to merge if the PR creator doesn't have commit access.
Furthermore, I looked at the code and I didn't see how we were handling the case where:
- Reviewer A has approved
- Reviewer B has requested changes
Do we currently comment with `! This PR is ready to merge !` when that's the case? If so, that seems wrong to me, since many PRs have an approval but are still waiting for some comments to be addressed before they can be merged.
https://github.com/llvm/llvm-project/pull/124910
More information about the llvm-commits
mailing list