Skip to content
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

Getting commit access #4246

Merged
merged 17 commits into from
Sep 3, 2024
Merged

Conversation

jonmeow
Copy link
Contributor

@jonmeow jonmeow commented Aug 23, 2024

Establish a process for getting commit access. We will:

  • Grant access based on a developer's commit history.
    • Someone with commit access should nominate, and a contributor may ask.
    • A lead will approve nominations. Only one lead is needed.
  • Remove commit access once someone is idle for 6 months.
    • "Idle" means no significant project activity on any of GitHub, Discord,
      or in meetings.
    • Access removed due to being idle will be restored on request.

@jonmeow jonmeow added proposal A proposal proposal draft Proposal in draft, not ready for review labels Aug 23, 2024
@jonmeow jonmeow force-pushed the proposal-groups branch 2 times, most recently from 074b5ac to 9a76c17 Compare August 23, 2024 20:00
@jonmeow jonmeow changed the title groups Getting commit access Aug 23, 2024
@jonmeow
Copy link
Contributor Author

jonmeow commented Aug 23, 2024

Note, looking at Commit acess:

  • camio - last comment Apr 2023, but has been active in other forums
  • chandlerc - active this week
  • fowles - last comment Oct 2023, no PRs
  • geoffromer - active this week
  • jonmeow - active this week
  • josh11b - active this week
  • jsiek - Sep 2022
  • mattgodbolt - Jul 2022
  • pixep - Aug 2023, though recently active on Discord
  • pk19604014 - Sep 2022
  • pmqtt - Feb 2023
  • tkoeppe - Sep 2021 for PR activity, Nov 2023 in discussions
  • wolffg - Aug 2022 for PR activity, active in discussions
  • zygoloid - active two weeks ago

So if we're basing this purely on PR activity, that would mean removing commit access for everyone but chandlerc, geoffromer, jonmeow, josh11b, and zygoloid.

@jonmeow jonmeow marked this pull request as ready for review August 23, 2024 20:16
@github-actions github-actions bot added documentation An issue or proposed change to our documentation proposal rfc Proposal with request-for-comment sent out and removed proposal draft Proposal in draft, not ready for review labels Aug 23, 2024
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly wording tweaks... but maybe @dwblaikie can also help with wording as he's been involved in similar discussions in the LLVM community?

docs/project/commit_access.md Outdated Show resolved Hide resolved
docs/project/commit_access.md Outdated Show resolved Hide resolved
docs/project/commit_access.md Outdated Show resolved Hide resolved
docs/project/commit_access.md Outdated Show resolved Hide resolved
docs/project/commit_access.md Outdated Show resolved Hide resolved
github-merge-queue bot pushed a commit that referenced this pull request Aug 28, 2024
I'm trying to use this just to document the de facto state. This is
linked to #4246, but I'm trying to keep the two arranged to allow
independent merges.
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. No real comments on the policy or approach, just some minor wording things.

I do worry a little that having people self-nominate or ask their reviewer for commit access might lead to some awkward conversations if we think they're not ready yet, and might also lead to some forms of self-nomination bias, but I don't have a better suggestion, so I'm happy to go with this.

Comment on lines 32 to 35
Note that developers with commit access are not required to do reviews or help
with approving and merging PRs. It can still be useful to allow authors to fix
trivial PR comments after approval and then merge their own PR, for example
because the review noted a minor typo, or to resolve merge conflicts.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not understand this paragraph. (For what it's worth, I think the rest of the text works fine without it.)

First sentence: is this saying

  1. Commit access is not accompanied with a responsibility to do reviews or help with approving and merging PRs. (Developers with commit access are not required to [do certain things] as a condition of their commit access)
    or
  2. Reviews and approving or merging PRs can be done without the help of a developer with commit access. (Developers with commit access are not required in order to [do certain things])

Second sentence: What are the consequences of this being useful? Is some particular action encouraged or discouraged, or some policy or advice we want to give here? How is this connected to what we said in the first sentence?

Also, can authors without commit access merge their own PRs after approval? I thought that merging PRs, even once approved, still requires commit access?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, there was some discussion about this paragraph here: #4246 (comment)

And...

Also, can authors without commit access merge their own PRs after approval? I thought that merging PRs, even once approved, still requires commit access?

I believe they cannot -- and I believe enabling this is a primary motivation for granting commit access.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Authors without commit access haven't ever been able to merge changes.

Also, recently actions became more restrictive for security reasons. I think in the past, a contributor only needed "approve and run" on their first PR. I'm not sure if there's any threshold where "approve and run" is no longer needed now.

So the present state is that people without commit access require assistance in order to both run actions and merge commits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, rephrased the paragraph again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! The new phrasing works for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, one small edit, "smooth over" -> "ease" due to possible connotations.

docs/project/commit_access.md Outdated Show resolved Hide resolved
docs/project/commit_access.md Outdated Show resolved Hide resolved
proposals/p4246.md Show resolved Hide resolved
Copy link
Contributor

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving for leads (and confirmed with all three FWIW, given the nature of this). Thanks!

@chandlerc chandlerc added this pull request to the merge queue Sep 3, 2024
Merged via the queue into carbon-language:trunk with commit 8b0154c Sep 3, 2024
8 checks passed
@jonmeow jonmeow deleted the proposal-groups branch September 4, 2024 15:31
dwblaikie pushed a commit to dwblaikie/carbon-lang that referenced this pull request Sep 6, 2024
Establish a process for getting commit access. We will:

-   Grant access based on a developer's commit history.
- Someone with commit access should nominate, and a contributor may ask.
    -   A lead will approve nominations. Only one lead is needed.
-   Remove commit access once someone is idle for 6 months.
- "Idle" means no significant project activity on any of GitHub,
Discord,
        or in meetings.
    -   Access removed due to being idle will be restored on request.

---------

Co-authored-by: Chandler Carruth <[email protected]>
dwblaikie pushed a commit to dwblaikie/carbon-lang that referenced this pull request Sep 9, 2024
Establish a process for getting commit access. We will:

-   Grant access based on a developer's commit history.
- Someone with commit access should nominate, and a contributor may ask.
    -   A lead will approve nominations. Only one lead is needed.
-   Remove commit access once someone is idle for 6 months.
- "Idle" means no significant project activity on any of GitHub,
Discord,
        or in meetings.
    -   Access removed due to being idle will be restored on request.

---------

Co-authored-by: Chandler Carruth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation An issue or proposed change to our documentation proposal rfc Proposal with request-for-comment sent out proposal A proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants