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
64 changes: 64 additions & 0 deletions docs/project/commit_access.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# Commit access

<!--
Part of the Carbon Language project, under the Apache License v2.0 with LLVM
Exceptions. See /LICENSE for license information.
SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-->

<!-- toc -->

## Table of contents

- [Overview](#overview)
- [Getting access](#getting-access)
- [Removing access](#removing-access)

<!-- tocstop -->

## Overview

Commit access is what allows developers to merge PRs into the main
[carbon-lang repository](https://github.com/carbon-language/carbon-lang/). It
also gives the ability to [review, approve, and merge changes](code_review.md).
jonmeow marked this conversation as resolved.
Show resolved Hide resolved

Developers with commit access are not required to do reviews. It is granted as a
convenience in order to allow fixing PRs after approval, for example because the
review noted a minor typo, or because there is a conflict that the PR author
needs to resolve before merging.

jonmeow marked this conversation as resolved.
Show resolved Hide resolved
Developers with commit access are expected to make reasonable choices about when
to review and approve PRs for others, and what changes to make to their own PRs
without asking for another round of review.
jonmeow marked this conversation as resolved.
Show resolved Hide resolved

## Getting access

Contributors can request commit access based on their commit history. We want to
make sure that developers with commit access are reasonably familiar with the
style and structure of the codebase. Access will typically be granted when
developers have contributed several PRs and are expecting to continue making
more.

Typically someone who has been doing reviews for a contributor will nominate,
and contributors are encouraged to ask a reviewer to nominate if needed.
Nominations will be reviewed by at least one lead, and a lead's approval will
lead to access.
jonmeow marked this conversation as resolved.
Show resolved Hide resolved

When approved, an invitation will be sent to join the
[Commit access team](https://github.com/orgs/carbon-language/teams/commit-access)
on GitHub. The invitation should cause a notification from GitHub, but it's also
possible to go to the
[invitation link](https://github.com/orgs/carbon-language/invitation) directly.
Access is granted when the invitation is accepted.

## Removing access

We'll periodically remove commit access from contributors who have been idle for
over 6 months. We'll use a combination of sources to determine what "idle"
means, including whether a developer has been either sending or reviewing PRs.

jonmeow marked this conversation as resolved.
Show resolved Hide resolved
For example, for `jonmeow`, GitHub searches (defaulting to PRs for convenience,
but other types are included):

- [repository:carbon-language/carbon-lang author:jonmeow](https://github.com/search?q=repository%3Acarbon-language%2Fcarbon-lang+author%3Ajonmeow&type=pullrequests)
- [repository:carbon-language/carbon-lang commenter:jonmeow](https://github.com/search?q=repository%3Acarbon-language%2Fcarbon-lang+commenter%3Ajonmeow&type=pullrequests)
112 changes: 112 additions & 0 deletions proposals/p4246.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
# Getting commit access

<!--
Part of the Carbon Language project, under the Apache License v2.0 with LLVM
Exceptions. See /LICENSE for license information.
SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-->

[Pull request](https://github.com/carbon-language/carbon-lang/pull/4246)

<!-- toc -->

## Table of contents

- [Abstract](#abstract)
- [Problem](#problem)
- [Background](#background)
- [Proposal](#proposal)
- [Rationale](#rationale)
- [Alternatives considered](#alternatives-considered)
- [Longer idle times](#longer-idle-times)

<!-- tocstop -->

## Abstract

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.
jonmeow marked this conversation as resolved.
Show resolved Hide resolved
- "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.

## Problem

Right now, we have an undocumented process for getting commit access, and no
real agreement for when to remove it. Commit access is important, so rather than
just making a documentation edit, I'm submitting this as a proposal.

## Background

When we say "commit access", what we mean is the ability to push commits to the
main `carbon-lang` repository, regardless of branch. Some details about the
implications:

- Review and approvals would remain required for pushes to `trunk`.
- This does grant access to push to other branches, although we will
continue to encourage fork-based workflows.
- Pushing PRs becomes easier with commit access.
- Action workflows will automatically execute, instead of requiring a
per-commit approval by someone with commit access.
- Modifying PRs after review approval is possible.
- This is both good (for small updates such as fixing typos and
resolving conflicts) and bad (particularly for code security).
- Communication delays can make it hard for an author to resolve conflicts
and reviewer to approve and merge before more conflicts are introduced.
- A possible solution to the conflict problem is to have the reviewer
merge PRs more frequently, and regardless of any decision here, we
may eventually need to adopt that approach.
- Commit access functionally means the ability to approve and merge PRs from
others.
- As alternatives, we could use either a separate GitHub team for
approvals or
[CODEOWNERS](https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-code-owners).
We have avoided these so far because we're small, having backup
approvers can be helpful, and mistakes are easy to undo.
- This does not include the ability to push to other repositories. While there
are a few in the `carbon-language` organization, only the `carbon-lang`
repository is actively used.
- People with commit access in effect have access to secrets, can make
releases, and so on.

## Proposal

The key things I think should be covered in this proposal are:

- Whether we want to require a lead to approve commit access additions.
- We're choosing to do a 6 month inactive period for removal.

Things I think we should be okay iterating on without going through evolution
include:

- Exactly how we define non-idle activity beyond merging and approving PRs,
both of which mechanically require this access.
- The detailed process for additions, including nominating.
- We want a good starting point, but it may not be worth sending all
changes through the proposal process.
- The detailed process for removals, such as whether leads are approving
removals.
- This is something it's been suggested to fully automate, preventing
review of removals.

See the new [Commit access](/docs/project/commit_access.md) document for
details.

## Rationale

- [Community and culture](/docs/project/goals.md#community-and-culture)
- Establishing the processes around commit access, and making sure they're
reasonable, is important to maintaining the community.

## Alternatives considered

### Longer idle times

We discussed using a longer idle time, like 1, 2, or 3 years. We're leaning
towards the shorter 6 month period because of concerns about forgotten access
causing issues. We're hoping 6 months is a minimum of inconvenience, and want it
to be easy to get access back on request.
Loading