[Workflows] Enable commit access requests via GitHub issues #100458
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,3 +27,6 @@ | |
|
|
||
| 'bolt': | ||
| - '/\bbolt(?!\-)\b/i' | ||
|
|
||
| 'infrastructure:commit-access-request': | ||
| - '/Request Commit Access/' | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -476,11 +476,12 @@ Obtaining Commit Access | |
| ----------------------- | ||
|
|
||
| We grant commit access to contributors with a track record of submitting high | ||
| 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.
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.
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.
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.
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| with your github username, and explain why you are requesting commit access in | ||
|
There was a problem hiding this comment.
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, 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:
etc. When someone doesn't provide any rationale and there isn't anything obvious, I ask them what their plan is. |
||
| the issue description. If approved, a GitHub invitation will be sent to your | ||
tstellar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| GitHub account. In case you don't get notification from GitHub, go to | ||
| `Invitation Link <https://github.com/orgs/llvm/invitation>`_ directly. Once | ||
| accept the invitation, you'll get commit access. | ||
jh7370 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a shorter label? Maybe
infra:commit-access.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have the
infrastructurelabel 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not insisting on
infrain 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, andinfrastructureeats a lot of space as a prefix.I can rename existing
infrastructurelabels toinfraif that's the only blocker.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.