-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[Workflows] Enable commit access requests via GitHub issues #100458
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
60ef4ac
[Workflows] Enable commit access requests via GitHub issues
tstellar beda0a3
Address Review Comments:
tstellar 840e6d0
Merge remote-tracking branch 'origin/main' into commit-access-request
tstellar 326ce28
Fix documentation build
tstellar 1180120
Fix documentation again
tstellar e4b6512
Update llvm/docs/DeveloperPolicy.rst
tstellar a0070b3
Update llvm/docs/DeveloperPolicy.rst
tstellar File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,3 +27,6 @@ | |
|
||
'bolt': | ||
- '/\bbolt(?!\-)\b/i' | ||
|
||
'infra:commit-access-request': | ||
- '/Request Commit Access/' |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -476,13 +476,13 @@ Obtaining Commit Access | |
----------------------- | ||
|
||
We grant commit access to contributors that can provide a valid justification. | ||
If you would like commit access, please send an email to | ||
`Chris <mailto:[email protected]>`_ with your GitHub username. This is true | ||
for former contributors with SVN access as well as new contributors. If | ||
approved, a GitHub invitation will be sent to your GitHub account. In case you | ||
don't get notification from GitHub, go to | ||
If you would like commit access, please use this `link <https://github.com/llvm/llvm-project/issues/new?title=Request%20Commit%20Access%20For%20%3Cuser%3E&body=%23%23%23%20Why%20Are%20you%20requesting%20commit%20access%20?>`_ to file | ||
an 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 | ||
the issue description. If approved, a GitHub invitation will be sent to your | ||
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. | ||
you accept the invitation, you'll get commit access. | ||
|
||
Prior to obtaining commit access, it is common practice to request that | ||
someone with commit access commits on your behalf. When doing so, please | ||
|
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!).
Uh oh!
There was an error while loading. Please reload this page.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.