[Workflows] Enable commit access requests via GitHub issues #100458
Conversation
This updates the auto-labeler to match a specific issue title that is going to be used for requesting commit access and then add the infrastructure:commit-access-request label. This will notify the admin team who will be able to handle the request. See https://discourse.llvm.org/t/rfc-change-the-process-for-requesting-commit-access/80184
llvm/docs/DeveloperPolicy.rst
Outdated
| don't get notification from GitHub, go to | ||
| quality patches. If you would like commit access, please use this `link | ||
| <https://github.com/llvm/llvm-project/issues/new?title=Request%20Commit%20Access%20For%20<user>&body=%23%23%23%20Why%20Are%20you%20requesting%20commit%20access%20?>`_ to file | ||
| and issue and request commit access. Replace the <user> string in the title |
There was a problem hiding this comment.
Should we ask for the list of merged PRs to simplify the review?
There was a problem hiding this comment.
Would the link get too complicated if we tried to link back to this document?
There was a problem hiding this comment.
Should we ask for the list of merged PRs to simplify the review?
This is not currently required, and I'm trying to be careful not to change the commit access requirements in this PR. I just want to change the process.
There was a problem hiding this comment.
Would the link get too complicated if we tried to link back to this document?
It's possible, can you give me an example of what you think the full issue body should look like?
There was a problem hiding this comment.
This is not currently required, and I'm trying to be careful not to change the commit access requirements in this PR. I just want to change the process.
Well, the policy is to have quality patches submitted. So, how we'd verify this? See e.g. #100457 (comment)
There was a problem hiding this comment.
@asl I know that's what the documentation says, but that's not what's been happening in practice. There has been a lot of disagreement over what the commit access requirements should be and I don't want to restart this discussion just yet. I want to improve the process, but also keep the status quo as far as commit access requirements for now.
There was a problem hiding this comment.
I think in any case if the reviewer wants to get this piece of data it’s a fairly easy search on GitHub UI.
.github/new-issues-labeler.yml
Outdated
| @@ -27,3 +27,6 @@ | |||
|
|
|||
| 'bolt': | |||
| - '/\bbolt(?!\-)\b/i' | |||
|
|
|||
| 'infrastructure:commit-access-request': | |||
There was a problem hiding this comment.
Can we use a shorter label? Maybe infra:commit-access.
There was a problem hiding this comment.
We already have the infrastructure label and I was trying to follow our convention of sub-labels. I think if we do s/infrastructure/infra/ here, then we should do it for all labels.
There was a problem hiding this comment.
I'm not insisting on infra in particular (even though I prefer it, and renaming labels is easy), but I do have general concerns over the length. Almost all the labels we have at the moment are at least as half as short of what is proposed here, and infrastructure eats a lot of space as a prefix.
I can rename existing infrastructure labels to infra if that's the only blocker.
There was a problem hiding this comment.
I'll update the label to use the short name. Note that if we change the name of existing lables without changing the names of the pr-subscribers or issues-subscriber teams.
| quality patches. If you would like commit access, please use this `link | ||
| <https://github.com/llvm/llvm-project/issues/new?title=Request%20Commit%20Access%20For%20<user>&body=%23%23%23%20Why%20Are%20you%20requesting%20commit%20access%20?>`_ to file | ||
| and issue and request commit access. Replace the <user> string in the title | ||
| with your github username, and explain why you are requesting commit access in |
There was a problem hiding this comment.
and explain why you are requesting commit access in the issue description.
Should we drop this bit? The explanation is always going to be awkward for the user to write because it's basically going to boil down to "it makes my life easier to commit on my own" or "someone asked me to request commit access"; it doesn't seem like an explanation is going to help the admin team make a determination because they still need to validate that the request is reasonable regardless of explanation. (If the admin team thinks a request requires further explanation, they can leave a comment asking for it, but that doesn't need to be a documented part of the process.)
There was a problem hiding this comment.
I don't disagree that the explanation may be awkward, but according to @lattner's description of how the current process works, users are asked for a justification.
I may be overly cautious here, but I've proposed changing the commit access requirements in the past, and since this proposal was rejected,
I'm trying really hard to not change or even give the impression of changing anything about the current process, because I don't want people to think I'm trying to backdoor changes into the process when that's not my intention.
There was a problem hiding this comment.
I don't have a strong opinion on this, but it is worth mentioning that reason I brought it up is because the current documented process does not require an explanation even if that's what Chris does in practice. It looked like a new requirement to me because the old docs don't mention it.
That said, I'm okay keeping it as well; I totally understand not wanting it to look like backdooring changes to the process (I don't think that's what's happening here!).
There was a problem hiding this comment.
I think it’s just that when writing an email to Chris you typically wouldn’t just ask for permission and give a bit of context on who you are/why you need it, which tends to be natural in email-to-one-person format. It probably doesn’t fundamentally matter, especially if you’re asking from GitHub where it’s easy to see who you are in terms of LLVM contributions. At the same time if you offer to the requester the option to add some context (with one of those default-text GitHub issues for example), most people would have no problem filling it up ("I have made X contributions to the project so far and I find that having commit access would help me maintain this part of the codebase more easily.")
There was a problem hiding this comment.
In practice, people include rationale for getting commit access, but it is all over the map. It varies from:
- Here's a number of things I've submitted.
- My manager at (bigco) told me to apply, they are often cc'd
- Someone told on a list told me to apply so I can land my patches.
etc. When someone doesn't provide any rationale and there isn't anything obvious, I ask them what their plan is.
* Shorten label name * Fix typos
|
I think I've addressed the most of the comments. Based on the discussion about whether or not to ask for a justification for commit access, would it make sense to first land a separate PR, that brings the documentation up to date with reality. e.g. |
Here is the PR to clarify the current commit access requirements: #101414 |
I've merged #101414 and updated this PR to include it now. |
|
Ping. Any other comments on this? |
|
Ping. |
1 similar comment
|
Ping. |
Co-authored-by: Vlad Serebrennikov <[email protected]>
Co-authored-by: Vlad Serebrennikov <[email protected]>
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/4737 Here is the relevant piece of the build log for the reference |
This updates the auto-labeler to match a specific issue title that is going to be used for requesting commit access and then add the infrastructure:commit-access-request label.
This will notify the admin team who will be able to handle the request.
See https://discourse.llvm.org/t/rfc-change-the-process-for-requesting-commit-access/80184