-
Notifications
You must be signed in to change notification settings - Fork 53
Contributing
Christopher Patton edited this page Mar 4, 2021
·
10 revisions
The goal of the guidelines below is to establish procedures for landing code in Cloudflare-Go that avoid unnecessary tension and never-ending reviews.
The following rules apply to all PRs.
- Each directory has a set of code owners (see CODEOWNERS). PRs require approval from at least two owners of each directory that is modified. (If the directory has just one code owner, then only that owner's approval is required.)
- We welcome comments from external collaborators, especially when they offer expertise we don't have. However, their approval alone is insufficient to land a PR. When there is a disagreement, code owners have the final say.
- Most commit messages have the form "package: Do a thing", e.g., "crypto/tls: Implement draft-ietf-tls-esni-08". The body of the message should summarize what the change does. (Have a look at the git log for examples.)
Most PRs are initiated by an Issue, though things like bug fixes and CIRCL updates may not need an Issue. New features, or re-design of existing ones, always require an Issue. The following rules apply to these PRs:
- The discussion of whether to include a new feature, what its requirements are, and how it should be designed are carried out in an Issue. Most of the time, much the design of the feature is prescribed by an RFC or Internet-Draft; when it's not, the assignee of the issue needs to come up with a design and get rough consensus on it.
- The Issue discussion also includes any changes to the public API. The assignee must get approval from codeowners for any such changes.
- A PR for a new feature is created only when there is rough consensus on how to proceed.
- The discussion in the comments of the PR must be limited to code design only; all other comments belong in the initiating Issue. (Bike-shedding on feature design when reviewing a PR is a recipe for disaster.)
A code owner is someone with subject matter expertise or who owns a service that depends on the code. Code owners have the following responsibilities.
- There is a lot of code in this repository about which none of us is an expert. We all own this code, so it's on all of us to provide careful and timely reviews. It will often be necessary to study the impacted code in order to ensure we understand how it's changed.
- An important consideration for reviews is long-term maintainability. In particular, it's important that changes minimize the difficulty of rebasing "cf" on upstream Go.
- With rare exceptions, new features need to be disabled by default. This is to prevent services that depend on Cloudflare-Go from changing in an unexpected way.
- Code owners need to respond promptly to Issues and PRs that impact their code.
Because code owners are accountable for any bugs or vulnerabilities that arise, it's imperative that owners understand the code they're merging. For this reason:
- Owners have the right to control the scope of a PR. If it can be reasonably decomposed into multiple discrete steps, and the timeline permits this, then the owner may ask this to be done.
- Owners set the standard for code clarity and readability. If the assignee or any other reviewer disagrees on a matter of code design or implementation details, owners have the final say.
- Owners decide if tests have adequate coverage.