DeveloperPolicy: Update commit access requirements #127006
Conversation
llvm/docs/DeveloperPolicy.rst
Outdated
| 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 | ||
| the issue description. Once the issue is created, you will need to get two | ||
| current contributors to ack your requests before commit access will be granted. |
There was a problem hiding this comment.
Maybe "to support" instead of "to ack"?
There was a problem hiding this comment.
Would it be too much to give a little hand here and say something like: "Providing evidence of your contributions would help reviewers support your case. This includes links to your pull requests, forum posts, etc."
There was a problem hiding this comment.
Would it be too much to give a little hand here and say something like: "Providing evidence of your contributions would help reviewers support your case. This includes links to your pull requests, forum posts, etc."
I think that risks giving the impression that more is required than having had 3 PRs merged, and there's not consensus on that.
I do think it would be helpful to give advice like "Once the issue is created, you will need to get two
current contributors to ack your requests before commit access will be granted. It is recommended that you CC reviewers of your merged patches in order to provide this approval."
There was a problem hiding this comment.
Would it be too much to give a little hand here and say something like: "Providing evidence of your contributions would help reviewers support your case. This includes links to your pull requests, forum posts, etc."
We have automation which posts a contribution summary when someone applies for commit access and notifies people who have reviewed or committed their patches. See #127077 (comment)
There was a problem hiding this comment.
I've been reflecting on this some more, and please consider this 100% non-blocking as I know it's been a long process...
I think it remains unclear what criteria the reviewer uses to give the 'ack'. If the sole requirement is that 3 patches were committed then it would be almost automated and surely one person double checking there was no mistake (e.g. checking for reverts) would be fine. But if more consideration is required, what is it? If one group of committers applies very different criteria to another, what do we do?
This is a step in the right direction, but it feels like we've not really succeeded in resolving the ambiguity of commit access requests unless that's clarified.
There was a problem hiding this comment.
@asb It's a good question. For me, the ack means 1) I know this person and 2) I believe they will adhere to our Developer Policy/Code of Conduct.
There was a problem hiding this comment.
I wonder if it would be uncontroversial to say something like:
"Reviewers of your committed patches will automatically be CCed upon creating the issue. Most commonly these reviewers will provide the necessary approval, but approvals from other LLVM committers are also acceptable. Those reviewing the application are confirming that you have indeed had three patches committed, and that based on interactions on those reviews and elsewhere in the LLVM community they have no concern about you adhering to our Developer Policy and Code of Conduct."
There was a problem hiding this comment.
"Reviewers of your committed patches will automatically be CCed upon creating the issue. Most commonly these reviewers will provide the necessary approval, but approvals from other LLVM committers are also acceptable. Those reviewing the application are confirming that you have indeed had three patches committed, and that based on interactions on those reviews and elsewhere in the LLVM community they have no concern about you adhering to our Developer Policy and Code of Conduct."
@asb Sounds good to me, I've added this paragraph to the document.
| We grant commit access to contributors that can provide a valid justification. | ||
| 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 |
There was a problem hiding this comment.
I'm a bit late to the party, but I would suggest something like the following introductory sentence:
LLVM operates using Github Pull Requests. In most cases, contributing to the project does not require commit access, as Pull Request reviewers can perform the merge once ready. However, contributors are eligible to obtain direct commit access to the repository once they have 3 or more merged pull requests. To do so, you may use this link <...>
This clarifies that one can actually go a long way without requiring commit access.
| on those reviews and elsewhere in the LLVM community they have no concern about you | ||
| adhering to our Developer Policy and Code of Conduct. | ||
|
|
||
| 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 | ||
| you accept the invitation, you'll get commit access. |
There was a problem hiding this comment.
I can't comment on the lines below directly, but the following paragraph should probably be updated too:
Prior to obtaining commit access, it is common practice to request that
someone with commit access commits on your behalf. When doing so, please
provide the name and email address you would like to use in the Author
property of the commit.
In fact, I think it can be removed entirely, but we may want to keep the bit of information about a valid author name and email address if we don't discuss it anywhere else.
| current contributors to support your request before commit access will be granted. | ||
|
|
||
| Reviewers of your committed patches will automatically be CCed upon creating the issue. | ||
| Most commonly these reviewers will provide the necessary approval, but approvals |
There was a problem hiding this comment.
As a reviewer, how am I supposed to provide approval? One of these issues has been assigned to me and I don't know what I'm supposed to do with it, even after reading this documentation and skimming the very long discourse thread.
There was a problem hiding this comment.
@jayfoad You can just leave a comment say " I support this request" or something similar.
See https://discourse.llvm.org/t/rfc-commit-access-criteria/84073