[llvm] [GitHub] Add workflows to manage merging of PRs from authors without commit access (PR #124910)

[David Spickett via llvm-commits]

DavidSpickett wrote:

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

Since it's only used for this workflow and removing it is a cheap way to remove PRs from the search results, that's why I remove it. I agree that there are uses where you might want to use it for queries. For instance, we could figure out how many PRs came from people without commit access in the last N months (but this is a different concern, I'm not using that to justify its use here).

I decided not to make it "needs-merge" or anything that implied that it *should* be merged just because it has the label. But yes it could be something specific to the workflow.

We can leave the label on and search the existing comments to see if we have pinged already, it's just a bit less neat in the implementation.

We can ditch the label completely and check the author's access each time we look at the PR, it's just more API calls. I preferred the label solution as it also added a hint visually, that might help inform reviewers should the workflow not cover some scenario.

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

Yes we could do that. You mean generally for any PR, with a unique message if they don't have commit access.

We can either continue to react to the first time we see approvals there, then delay pings after that, or we can delay the initial ping as well.

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

The `! This PR...` is from an earlier version of this that was more a call and response setup. The author would write that comment to tell the workflow to ping the reviewers at that moment.

The version in the PR now is simplified and does not require this.

You are correct that I have not handled that case. Of all the most recent reviews from each reviewer, I need to check that they are all approvals or comments, and none are requesting changes.

Thanks, I wouldn't have spotted that, not until production anyway :)

We could go further and require that all open comments are resolved, but this is less intuitive if you are the author. Might need the workflow itself to comment "this could be merged but for X and Y". This is also not part of the general llvm flow at this time, plenty of PRs get merged with unresolved comments. They may have actually been addressed in the code, but no one clicked the button on GitHub.

Which is maybe something to address later but I'm trying not to change policy as part of this work.

I think reframing this as "ping approved but unmerged PRs after N days", later followed by "customise that ping for those without commit access" could be a path forward. As I do like the idea of extending this to all PRs.

https://github.com/llvm/llvm-project/pull/124910