From 3af3e1ad3d601173f4bc012553eaec54ff38f5f6 Mon Sep 17 00:00:00 2001 From: Mosh <1306020+mishmosh@users.noreply.github.com> Date: Thu, 7 Feb 2019 12:48:29 -0500 Subject: [PATCH 1/4] [WIP comments welcome] Maintainers and committers Fixes #516. Not all decisions finalized, but it's time to move the proposal into PR. Comments welcome. * Small changes in "Code Reviews" section. * Add new "Roles" section. --- CONTRIBUTING.md | 62 +++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2bf4c7a743..7a2191c2c0 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -24,6 +24,7 @@ Filecoin, including go-filecoin and all related modules, follows the * [Gotchas](#gotchas) * [The Spec](#the-spec) * [What is the bar for inclusion in master?](#what-is-the-bar-for-inclusion-in-master) +* [Roles](#roles) ## How can I contribute? @@ -58,12 +59,14 @@ Check out the [Go-Filecoin code overview](CODEWALK.md) for a brief tour of the c ### Code Reviews -`go-filecoin` requires 2 approvals for all PRs, with at least one of {@phritz, @acruikshank, @whyrusleeping}. If your PR hasn't been reviewed in 3 days, pinging reviewers via Github or community chat is also welcome and encouraged. +`go-filecoin` requires 2 approvals for all PRs. We use the following process, which aims to merge code quickly and efficiently, while avoiding both accidental and malicious introduction of bugs, unintended consequences, or vulnerabilities. + +The first review is done by a committer. Once they deem it ready for merge, they will assign a maintainer for the second review. Once the PR has 2 Approvals and blocking comments have been addressed, the committer rebases and merges the PR. If your PR hasn't been reviewed in 3 days, pinging reviewers via Github or community chat is also welcome and encouraged. We use the following conventions for code reviews: - "Approve" means approval. If there are comments and approval, it is expected that you'll address the comments before merging. Ask for clarification if you're not sure. - - *Example: reviewer points out an off by one error in a blocking comment, but Approves the PR. Reviewee must fix the error, but the reviewer trusts you to do that.* + - *Example: reviewer points out an off by one error in a blocking comment, but Approves the PR. Reviewee must fix the error, but PR can progress to maintainer review. Committer confirms this before merge.* - "Request Changes" means you don't have approval, and the reviewer wants another look. - *Example: the whole design of an abstraction is wrong and reviewer wants to see it reworked.* @@ -244,3 +247,58 @@ Presently (Q1'19) the minimum bar is: * Lint (`go run ./build/*.go lint`). * Must match a subset of the spec. * Documentation is up to date. + +### Roles + +There are three main roles for people participating in `go-filecoin`. Each has a specific set of abilities and responsibilities: Contributors, Committers, and Maintainers. + +#### Contributors + +Anyone is welcome. If you have created an issue, commented on an issue or discussion forum thread, or submitted a PR, you are a contributor. + +Responsibilities: + +* TODO + +Abilities: + +* Open issues and PRs +* Comment on issues and PRs + +#### Committers + +A **Committer** is someone with a broad understanding of the codebase and the judgment and humility to call on others' expertise when needed. They have a consistent track record of quality contributions, regular participation, and enabling others. + +Responsibilities: + +* Review PRs and guide work to ready-to-merge +* With maintainer approval, rebase and merge contributor PRs +* Issue triage and other project stewardship + +Abilities: + +* Manage issues +* Merge PRs + +#### Maintainers + +A **Maintainer** is someone: + +1. **who is invested in and broadly familiar with the project**, as demonstrated by a history of significant technical-, process-, and/or project-level contributions; +2. **who deeply understands the system**, especially knowing when and who to defer to as a reviewer, and with an eye towards unintended consequences; +3. **who is actively engaged in project progress and stewardship** by enabling others through project-wide planning, code reviews, design feedback, etc.; and +4. **who is a model of trustworthiness, technical judgement, civility, and helpfulness**. + +Responsibilities: + +* Review: Timely, friendly review of PRs and design docs to ensure high-quality code and grow knowledge of committers and frequent contributors +* Planning and Improvements: Participate meaningfully in technical and process-related improvements at the project level +* Make significant, direct technical contributions +* Backstop for hard problems and general project stewardship (TODO: improve wording) + +Abilities: + +* Manage issues +* Merge PRs + +**Becoming a committer or maintainer:** Anyone can nominate someone for committership or maintainership by filing an issue {TODO: link to correct repo} pointing to evidence that the candidate (1) meets the definition and (2) is already performing the responsibilities decsribed in Roles. Existing maintainers must unanimously approve the new candidate. Removing a committer or maintainer requires either self-nomination, or confirmation by at least 66% of existing maintainers. From 8413fdf289d008eed6d4208f441178e9239e234b Mon Sep 17 00:00:00 2001 From: Mosh <1306020+mishmosh@users.noreply.github.com> Date: Fri, 8 Feb 2019 13:57:47 -0500 Subject: [PATCH 2/4] gently flag that some PRs need 3 approves --- CONTRIBUTING.md | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7a2191c2c0..053f007c16 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -47,7 +47,7 @@ Check out the [Go-Filecoin code overview](CODEWALK.md) for a brief tour of the c ### Design Before Code - Write down design intent before writing code, and subject it to constructive feedback. -- Major changes should have a [Design Doc](designdocs.md). +- Major changes should have a [Design Doc](https://github.com/filecoin-project/designdocs). - For minor changes, file an issue for your change if it doesn't yet have one, and outline your implementation plan on the issue. ### Pull Requests @@ -59,9 +59,11 @@ Check out the [Go-Filecoin code overview](CODEWALK.md) for a brief tour of the c ### Code Reviews -`go-filecoin` requires 2 approvals for all PRs. We use the following process, which aims to merge code quickly and efficiently, while avoiding both accidental and malicious introduction of bugs, unintended consequences, or vulnerabilities. +`go-filecoin` requires 2 approvals for all PRs. We use the following process, which aims to merge code quickly and efficiently while avoiding both accidental and malicious introduction of bugs, unintended consequences, or vulnerabilities. -The first review is done by a committer. Once they deem it ready for merge, they will assign a maintainer for the second review. Once the PR has 2 Approvals and blocking comments have been addressed, the committer rebases and merges the PR. If your PR hasn't been reviewed in 3 days, pinging reviewers via Github or community chat is also welcome and encouraged. +The first review is done by a committer. Once they deem it ready for merge, they will assign a maintainer for the second review. Once the PR has 2 Approvals and blocking comments have been addressed, the committer rebases and merges the PR. + +If your PR hasn't been reviewed in 3 days, pinging reviewers via Github or community chat is also welcome and encouraged. We use the following conventions for code reviews: @@ -70,8 +72,9 @@ We use the following conventions for code reviews: - "Request Changes" means you don't have approval, and the reviewer wants another look. - *Example: the whole design of an abstraction is wrong and reviewer wants to see it reworked.* -- [There is a proposal in progress to invert BLOCKING, to match most contributors' expectations.] By default, code review comments are advisory: the reviewee should consider them but doesn't _have_ to respond or address them. Comments that start with "BLOCKING" must be addressed and responded to. If a reviewer makes a blocking comment but does not block merging (by marking the review "Add Comments" or "Approve") then the reviewee can merge if the issue is addressed. +- By default, code review comments are advisory: the reviewee should consider them but doesn't _have_ to respond or address them. Comments that start with "BLOCKING" must be addressed and responded to. If a reviewer makes a blocking comment but does not block merging (by marking the review "Add Comments" or "Approve") then the reviewee can merge if the issue is addressed. +In rare cases, a maintainter may request approval from all maintainers for a wide-reaching PR. #### Code Reviewer Responsibilities: From d189844e271ef5b2e86ee9d10e8cc041d31c7acb Mon Sep 17 00:00:00 2001 From: Mosh <1306020+mishmosh@users.noreply.github.com> Date: Fri, 8 Feb 2019 15:01:19 -0500 Subject: [PATCH 3/4] add collaborators role --- CONTRIBUTING.md | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 053f007c16..0a447d2a3a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -253,7 +253,7 @@ Presently (Q1'19) the minimum bar is: ### Roles -There are three main roles for people participating in `go-filecoin`. Each has a specific set of abilities and responsibilities: Contributors, Committers, and Maintainers. +There are four main roles for people participating in `go-filecoin`. Each has a specific set of abilities and responsibilities: Contributors, Collaborators, Committers, and Maintainers. #### Contributors @@ -261,13 +261,25 @@ Anyone is welcome. If you have created an issue, commented on an issue or discus Responsibilities: -* TODO +* Participate in the project, following the [Code of Conduct](https://github.com/filecoin-project/community/blob/master/CODE_OF_CONDUCT.md). Abilities: * Open issues and PRs * Comment on issues and PRs +#### Collaborators + +A **Collaborator** is someone who demonstrates helpful contributions to the project. + +Responsibilities: + +* Make helpful contributions via issues, PRs, and other venues + +Abilities: +* Write to the repo (but cannot merge to master) +* Manage issues + #### Committers A **Committer** is someone with a broad understanding of the codebase and the judgment and humility to call on others' expertise when needed. They have a consistent track record of quality contributions, regular participation, and enabling others. From af16d758f160c4c10a02af8a27c59d96ea24aeda Mon Sep 17 00:00:00 2001 From: Mosh <1306020+mishmosh@users.noreply.github.com> Date: Fri, 8 Feb 2019 23:04:00 -0500 Subject: [PATCH 4/4] specify reviewer familiarity --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0a447d2a3a..417e8d9f58 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -61,7 +61,7 @@ Check out the [Go-Filecoin code overview](CODEWALK.md) for a brief tour of the c `go-filecoin` requires 2 approvals for all PRs. We use the following process, which aims to merge code quickly and efficiently while avoiding both accidental and malicious introduction of bugs, unintended consequences, or vulnerabilities. -The first review is done by a committer. Once they deem it ready for merge, they will assign a maintainer for the second review. Once the PR has 2 Approvals and blocking comments have been addressed, the committer rebases and merges the PR. +The first review is done by a committer familiar with that area of the codebase. Once they deem it ready for merge, they will assign a maintainer for the second review. Once the PR has 2 Approvals and blocking comments have been addressed, the committer rebases and merges the PR. If your PR hasn't been reviewed in 3 days, pinging reviewers via Github or community chat is also welcome and encouraged.