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

Remove CODEOWNERS #1367

Merged
merged 14 commits into from
Jul 15, 2022
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
jonmeow marked this conversation as resolved.
Show resolved Hide resolved
jonmeow marked this conversation as resolved.
Show resolved Hide resolved
- 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
jonmeow marked this conversation as resolved.
Show resolved Hide resolved

# 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'
zygoloid marked this conversation as resolved.
Show resolved Hide resolved

- 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):
Comment on lines +123 to +124
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still want to suggest this? I've not really seen people using it recently, and my experience trying it previously was not all that pleasant -- to the point that I find I prefer to delay reviews of dependent changes until the base change lands. It also seems exclusionary to provide a different workflow for people with commit access versus everyone else, especially if we do not intend for commit access to be granted to all regular contributors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally, I'd support removing it, but IIRC @chandlerc added it so it may be best to discuss with him.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, remove it. It just doesn't work. I've switched to just documenting in stacked PRs which commit range to look at and giving up once merges are occuring.


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