Skip to content

Commit

Permalink
Remove CODEOWNERS (#1367)
Browse files Browse the repository at this point in the history
Remove CODEOWNERS and rely on repository commit access

Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Co-authored-by: Richard Smith <richard@metafoo.co.uk>
Co-authored-by: Geoff Romer <gromer@google.com>
  • Loading branch information
4 people authored Jul 15, 2022
1 parent 6c291ba commit 8dd878b
Show file tree
Hide file tree
Showing 8 changed files with 356 additions and 27 deletions.
20 changes: 20 additions & 0 deletions .github/assign_prs/explorer.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# 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

# Set to true to add reviewers to pull requests
addReviewers: true

# Set to true to add assignees to pull requests
addAssignees: false

# A list of reviewers to be added to pull requests (GitHub user name)
reviewers:
- geoffromer
- jonmeow
- jsiek
- zygoloid

# A number of reviewers added to the pull request
# Set 0 to add all the reviewers (default: 0)
numberOfReviewers: 1
20 changes: 20 additions & 0 deletions .github/assign_prs/fallback.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# 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

# Set to true to add reviewers to pull requests
addReviewers: true

# Set to true to add assignees to pull requests
addAssignees: false

# A list of reviewers to be added to pull requests (GitHub user name)
reviewers:
- chandlerc
- jonmeow
- josh11b
- zygoloid

# A number of reviewers added to the pull request
# Set 0 to add all the reviewers (default: 0)
numberOfReviewers: 1
19 changes: 19 additions & 0 deletions .github/assign_prs/leads.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# 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

# Set to true to add reviewers to pull requests
addReviewers: true

# Set to true to add assignees to pull requests
addAssignees: false

# A list of reviewers to be added to pull requests (GitHub user name)
reviewers:
- KateGregory
- chandlerc
- zygoloid

# A number of reviewers added to the pull request
# Set 0 to add all the reviewers (default: 0)
numberOfReviewers: 1
19 changes: 19 additions & 0 deletions .github/assign_prs/toolchain.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# 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

# Set to true to add reviewers to pull requests
addReviewers: true

# Set to true to add assignees to pull requests
addAssignees: false

# A list of reviewers to be added to pull requests (GitHub user name)
reviewers:
- chandlerc
- jonmeow
- zygoloid

# A number of reviewers added to the pull request
# Set 0 to add all the reviewers (default: 0)
numberOfReviewers: 1
56 changes: 56 additions & 0 deletions .github/workflows/assign_prs.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
# 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

name: 'Auto Assign'
on:
pull_request:
types: [opened, ready_for_review]

jobs:
assign_reviewer:
runs-on: ubuntu-latest
steps:
- id: path-filter
uses: dorny/paths-filter@v2
with:
filters: |
leads:
- '*.md'
- 'LICENSE'
- 'docs/project/principles/*'
- 'docs/project/evolution.md'
- 'docs/project/goals.md'
- 'docs/project/roadmap.md'
- 'proposals/*.md'
explorer:
- 'explorer/**'
toolchain:
- 'toolchain/**'
- id: assign-leads
if: steps.changes.outputs.leads == 'true'
uses: shufo/auto-assign-reviewer-by-files@v1.1.3
with:
configuration-path: '.github/assign_prs/leads.yaml'

- id: assign-explorer
if: steps.changes.outputs.explorer == 'true'
uses: shufo/auto-assign-reviewer-by-files@v1.1.3
with:
configuration-path: '.github/assign_prs/explorer.yaml'

- id: assign-toolchain
if: steps.changes.outputs.toolchain == 'true'
uses: shufo/auto-assign-reviewer-by-files@v1.1.3
with:
configuration-path: '.github/assign_prs/toolchain.yaml'

- id: assign-fallback
if:
steps.changes.outputs.leads != 'true' &&
steps.changes.outputs.explorer != 'true' &&
steps.changes.outputs.toolchain != 'true'
uses: shufo/auto-assign-reviewer-by-files@v1.1.3
with:
configuration-path: '.github/assign_prs/fallback.yaml'
55 changes: 32 additions & 23 deletions docs/project/code_review.md
Original file line number Diff line number Diff line change
Expand Up @@ -72,22 +72,31 @@ partial review can be a good way to start contributing to Carbon. Contributors
with specific domain expertise or familiarity should also try to provide review
on changes touching relevant parts of the project.

Additionally, at least one _code owner_ of any file changed needs to review that
change. The code owners and what files they are responsible for are defined
using the
[`CODEOWNERS`](https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners#codeowners-syntax)
file in the root of the repository. Pull requests will automatically request
reviewers based on this file and enforce that these reviews take place.

While we do encourage people interested in contributing to Carbon by reviewing
changes to do so, we also suggest not overloading a single review. It can be
daunting for the author of a change to get feedback from a large number of
reviewers, and so we suggest keeping the number of reviewers reasonably small.
Additionally, at least one developer with commit access must review each change.
In Carbon, developers will focus on particular areas, loosely broken down as:

- [Carbon leads](groups.md#carbon-leads): [proposals](/proposals/) and other
important project documents, including the [Main README](/README.md),
[Code of Conduct](/CODE_OF_CONDUCT.md), [license](/LICENSE), and
[goals](goals.md).

- [Implementation team](groups.md#implementation-team): general changes.

- We split out auto-assignment by
[explorer](/.github/assign_prs/explorer.yaml),
[toolchain](/.github/assign_prs/toolchain.yaml), and
[other files, including documentation](/.github/assign_prs/fallback.yaml).

[Auto-assignment](/.github/workflows/assign_prs.yaml) will help find owners, but
won't always be perfect -- developers may take a PR they weren't auto-assigned
in order to help review go quickly. Contributors can also request multiple
reviewers, but it can be daunting to get feedback from a large number of
reviewers, so we suggest keeping the number of reviewers reasonably small.

Any reviews that explicitly request changes should be addressed, either with the
changes or an explanation of why not, before a pull request is merged. Further,
any code owners who have requested changes should explicitly confirm they're
happy with the resolution before the change is merged.
any owners who have requested changes should explicitly confirm they're happy
with the resolution before the change is merged.

When a team gives an affirm decision on an [evolution proposal](evolution.md),
each team member should explicitly note any of their comments on the pull
Expand Down Expand Up @@ -116,13 +125,13 @@ mechanical best practices to most effectively navigate them.
- Make any in-file comments needed, but add them to a pending review
rather than sending them directly.
- Finish the review and add any top-level review comments there.
- If you are a code owner who will be providing approval for the change, then
make sure to mark a review as requesting changes when you want the author to
- If you are an owner who will be providing approval for the change, then make
sure to mark a review as requesting changes when you want the author to
begin addressing your comment. Only use the "comment" review state if you
are still in the process of reviewing and don't expect the author to begin
working on further changes.
- If you are not a code owner asked to approve, use the difference between
a comment and requesting a change to help the author know whether to
- If you are not an owner asked to approve, use the difference between a
comment and requesting a change to help the author know whether to
circle back with you before landing the pull request if the relevant
owner(s) approve it.
- Don't reply to in-file comment threads in the conversation view, or with
Expand Down Expand Up @@ -400,9 +409,9 @@ in GitHub, but also state this explicitly in the message since this is the
default and doesn't indicate that your feedback _is_ addressed. For example, say
that "my comments are addressed, but leaving the final review to others" to
clearly indicate that you're happy but are deferring the decision to others. If
you are a code owner and deferring to someone else, it is essential to suggest
specific other reviewers. Otherwise, we risk all the code owners assuming
another is going to approve the change.
you are an owner and deferring to someone else, it is essential to suggest
specific other reviewers. Otherwise, we risk all the owners assuming another is
going to approve the change.

An important technique to make progress, especially with different working hours
and timezones, is to approve changes even with outstanding comments. For
Expand Down Expand Up @@ -454,7 +463,7 @@ There are two techniques to use to resolve these situations that should be tried
early on:

1. Bring another person into the review to help address the specific issue.
Typically they should at least be a code owner, and may usefully be a
Typically they should at least be an owner, and may usefully be a
[Carbon lead](groups.md#carbon-leads).

2. Ask the specific question in a broader forum, such as Discord, in order to
Expand All @@ -464,8 +473,8 @@ The goal of these steps isn't to override the author or the reviewer, but to get
more perspectives and voices involved. Often this will clarify the issue and its
trade-offs, and provide a simple resolution that all parties are happy with.
However, in some cases, the underlying conflict isn't actually addressed. While
there is a desire to generally bias towards the direction of the code owners
during reviews, reviews should _not_ turn into a voting process. The reason for
there is a desire to generally bias towards the direction of the owners during
reviews, reviews should _not_ turn into a voting process. The reason for
proceeding in a specific direction should always be explained sufficiently that
all parties on the review are satisfied by the explanation and don't feel the
need to escalate.
Expand Down
6 changes: 2 additions & 4 deletions docs/project/pull_request_workflow.md
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ Specifically, one pull request cannot serve as the _base_ for another pull
request, so each pull request will include all of the commits and diffs of the
preceding pull requests in the stack.

We suggest a specific workflow to address this:
We suggest a specific workflow to address this (note, commit access is
required):

1. Create your initial pull request from a branch of your fork, nothing special
is needed at this step. Let's say you have a branch `feature-basic` in your
Expand Down Expand Up @@ -158,9 +159,6 @@ We suggest a specific workflow to address this:
be force pushed as necessary and deleted. These branch names should only be
used for this ephemeral purpose. All other branch names are protected.
If you don't yet have this permission, just ask an [admin](groups.md#admins)
for help.

3. Create your stacked branch on your fork:
```shell
Expand Down
Loading

0 comments on commit 8dd878b

Please sign in to comment.