Skip to content

Commit

Permalink
governance: new change approval process to include approvers/reviewers
Browse files Browse the repository at this point in the history
Signed-off-by: Jared Watts <[email protected]>
  • Loading branch information
jbw976 committed Jun 17, 2019
1 parent 8da2221 commit d9f11a1
Show file tree
Hide file tree
Showing 4 changed files with 124 additions and 10 deletions.
37 changes: 37 additions & 0 deletions CODE-OWNERS
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
# Code ownership file that specifies approvers and reviewers for all areas of the Rook codebase.
# all: These members have the given role for all areas of the codebase.
# common: The rook.io common types, specs, logic, etc.
# build: The top level makefile, as well as the scripts and makefiles in the `build` directory.
# All other areas are for specific storage providers and covers all portions of code that is specific to their project.

areas:
all:
approvers:
- travisn
- galexrt
- jbw976
common:
approvers:
- bassam
build:
approvers:
- bassam
ceph:
approvers:
- BlaineEXE
- leseb
reviewers:
- noahdesu
edgefs:
approvers:
- dyusupov
reviewers:
- sabbot
cassandra:
approvers:
- yanniszark
nfs:
reviewers:
- rohan47
cockroachdb:
minio:
72 changes: 72 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -105,3 +105,75 @@ The first line is the subject and should be no longer than 70 characters, the
second line is always blank, and other lines should be wrapped at 80 characters.
This allows the message to be easier to read on GitHub as well as in various
git tools.

## Change Approval

The Rook project aims to empower contributors to approve and merge code changes autonomously.
The maintainer team does not have sufficient resources to fully review and approve all proposed code changes, so trusted members of the community are given these abilities according to the process described in this section.
The goal of this process is to increase the code velocity of all storage providers and streamline their day to day operations such as pull request approval and merging.

### Change Approval Roles

The model for approving changes is largely based on the [Kubernetes code review process](https://github.com/kubernetes/community/blob/master/contributors/guide/owners.md#code-review-using-owners-files),
where a set of roles are defined for different portions of the code base and have different responsibilities:

* **Reviewers** are able to review code for quality and correctness on some part of the project, but cannot merge changes.
* **Approvers** are able to both review and approve code contributions. While code review is focused on code quality and correctness, approval is focused on holistic acceptance of a contribution. Approvers can merge changes.

Both of these roles will require a time commitment to the project in order to keep the change approval process moving forward at a reasonable pace.
When automation is implemented to auto assign members to review pull requests, it will be done in a round-robin fashion, so all members must be able to dedicate the time needed.

Note that neither of these roles have voting powers in conflict resolution, these roles are for the code base change approval process only.

### Pull Request Flow

The general flow for a pull request approval process is as follows:

1. Author submits the pull request
1. Reviewers and approvers for the applicable code areas review the pull request and provide feedback that the author integrates
1. Reviewers and/or approvers signify their LGTM on the pull request
1. An approver approves the pull request based on at least one LGTM from the previous step
1. Note that the approver can heavily lean on the reviewer for examining the pull request at a finely grained detailed level. The reviewers are trusted members and approvers can leverage their efforts to reduce their own review burden.
1. An approver merges the pull request into the target branch (master, release, etc.)

### Role Assignments

#### Declarations

All roles will be assigned by the usage of [`CODE-OWNERS`](CODE-OWNERS) files committed to the code base.
These assignments will be initially be defined in a single file at the root of the repo and it will describe all assigned roles for the entire code base.
As we incorporate automation (i.e. bots) into this change acceptance process in the future, we can reorganize this initial single owners file into separate files amongst the codebase as the automation necessitates.

The format of the file can start with simply listing the reviewers and approvers for areas of the code base using a YAML format:

```yaml
areas:
feature-foo:
approvers:
- alice
- bob
reviewers:
- carol
```
#### Update Process
The process for adding or removing reviewers/approvers is described in the [project governance](GOVERNANCE.md#updating-change-approval-roles).
### Permissions
Role assignees will be made part of the following Rook organization teams with the given permissions:
* **Reviewers:** added to a new Reviewers team so they have write permissions to the repo to assign issues, add labels to issues, add issues to milestones and projects, etc. but cannot merge to protected branches such as `master` and `release-*`.
* **Approvers:** added to a new Approvers team that will have access to merge to protected branches.

### Automation

This process can be further improved by automation and bots to automatically assign the PR to reviewers/approvers, add labels to the PR, and merge the PR.
We should explore this further with some experimentation and potentially leveraging what Kubernetes has done, but automation isn’t strictly required to adopt and implement this model.

### Alternatives Considered

The built in support in Github for [`CODEOWNERS`](https://help.github.com/en/articles/about-code-owners) files was considered.
However, this only supports the automated assignment of reviewers to pull requests.
It has no tiering or differentiation between roles like the proposed approvers/reviewers model has and is therefore not a good fit.
21 changes: 11 additions & 10 deletions GOVERNANCE.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

This document defines governance policies for the Rook project.

## Roles
## Maintainer Roles

Rook uses a two-tiered system of maintainer roles:

Expand Down Expand Up @@ -95,22 +95,23 @@ the maintainers per the voting process below.
* In general continue to be willing to spend at least 25% of ones time working on Rook (~1.25
business days per week).

### Approving PRs

PRs may be merged after receiving at least **1 approval from a maintainer** (either senior or standard)
that is **not the author** of the PR, and preferably from a **different organization** than the PR author.
As complexity of a PR increases, such as design changes or major PRs, the need for an approval from
a different organization also increases. This should be a judgement call from the maintainers,
and it is expected that all maintainers act in good faith to seek approval from a different
organization when appropriate.

### Github Project Administration

Maintainers will be added to the Rook GitHub organization (if they are not already) and added to
the GitHub Maintainers team.

After 6 months, **senior** maintainers will be made an "owner" of the Rook GitHub organization.

## Updating Change Approval Roles

The full change approval process is described in the [contributing guide](CONTRIBUTING.md#change-approval).

All new reviewers and approvers must be nominated by someone (anyone) opening a pull request that adds the nominated person’s name to the appropriate [`CODE-OWNERS`](CODE-OWNERS) files in the appropriate roles.
Similarly, to remove a reviewer or approver, a pull request should be opened that removes their name from the appropriate [`CODE-OWNERS`](CODE-OWNERS) files.

The maintainer team will approve this update in the standard voting and conflict resolution process.
Note that new reviewer/approver nominations do not go through the standard pull request approval described in the [contributing guide](CONTRIBUTING.md#change-approval), only the maintainer team can approve updates of members to the change approval roles.

## Conflict resolution and voting

As previously mentioned, senior maintainers receive **2 votes** each and standard maintainers
Expand Down
4 changes: 4 additions & 0 deletions OWNERS.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ Maintainers will be added according to the [process for adding a maintainer](GOV

There are currently no emeritus maintainers. This list will be updated according to the [process for removing a maintainer](GOVERNANCE.md#removing-a-maintainer) in the [governance](GOVERNANCE.md).

## Code Owners

Code owners, as described in the [change approval process](GOVERNANCE.md#change-approval) of the governance, can be found in [`CODE-OWNERS`](CODE-OWNERS) files.

## Friends of Rook

This section lists people that are not currently maintainers but have demonstrated value to the community.
Expand Down

0 comments on commit d9f11a1

Please sign in to comment.