From 6af4303aae558c3b8bf658b53b37d25bff4d26c5 Mon Sep 17 00:00:00 2001 From: Jamey Sharp Date: Fri, 5 May 2023 07:41:54 -0700 Subject: [PATCH] Document our PR reviewer auto-assignment expectations (#6346) --- docs/SUMMARY.md | 4 +- docs/contributing-code-review.md | 117 +++++++++++++++++++++ docs/contributing-maintainer-guidelines.md | 5 + 3 files changed, 125 insertions(+), 1 deletion(-) create mode 100644 docs/contributing-code-review.md create mode 100644 docs/contributing-maintainer-guidelines.md diff --git a/docs/SUMMARY.md b/docs/SUMMARY.md index ffffc50a4ab6..f5e6bad15e7a 100644 --- a/docs/SUMMARY.md +++ b/docs/SUMMARY.md @@ -60,7 +60,9 @@ - [Cross Compiling](./contributing-cross-compiling.md) - [Coding Guidelines](./contributing-coding-guidelines.md) - [Development Process](./contributing-development-process.md) - - [Release Process](./contributing-release-process.md) - [Implementing Wasm Proposals](./contributing-implementing-wasm-proposals.md) + - [Maintainer Guidelines](./contributing-maintainer-guidelines.md) + - [Code Review](./contributing-code-review.md) + - [Release Process](./contributing-release-process.md) - [Governance](./contributing-governance.md) - [Code of Conduct](./contributing-coc.md) diff --git a/docs/contributing-code-review.md b/docs/contributing-code-review.md new file mode 100644 index 000000000000..c44de6a62527 --- /dev/null +++ b/docs/contributing-code-review.md @@ -0,0 +1,117 @@ +# Code Review + +We only merge changes submitted as GitHub Pull Requests, and only after they've +been approved by at least one Core Team reviewer who did not author the PR. This +section covers expectations for the people performing those reviews. These +guidelines are in addition to expectations which apply to everyone in the +community, such as following the Code of Conduct. + +It is our goal to respond to every contribution in a timely fashion. Although we +make no guarantees, we aim to usually provide some kind of response within about +one business day. + +That's important because we appreciate all the contributions we receive, made by +a diverse collection of people from all over the world. One way to show our +appreciation, and our respect for the effort that goes into contributing to this +project, is by not keeping contributors waiting. It's no fun to submit a pull +request and then sit around wondering if anyone is ever going to look at it. + +That does not mean we will review every PR promptly, let alone merge them. Some +contributions have needed weeks of discussion and changes before they were ready +to merge. For some other contributions, we've had to conclude that we could not +merge them, no matter how much we appreciate the effort that went into them. + +What this does mean is that we will communicate with each contributor to set +expectations around the review process. Some examples of good communication are: + +- "I intend to review this but I can't yet. Please leave me a message if I + haven't responded by (a specific date in the near future)." + +- "I think (a specific other contributor) should review this." + +- "I'm having difficulty reviewing this PR because of (a specific reason, if + it's something the contributor might reasonably be able to help with). Are you + able to change that? If not, I'll ask my colleagues for help (or some other + concrete resolution)." + +If you are able to quickly review the PR, of course, you can just do that. + +You can find open Wasmtime pull requests for which your review has been +requested with this search: + + + +## Auto-assigned reviewers + +We automatically assign a reviewer to every newly opened pull request. We do +this to avoid the problem of diffusion of responsibility, where everyone thinks +somebody else will respond to the PR, so nobody does. + +To be in the pool of auto-assigned reviewers, a Core Team member must commit to +following the above goals and guidelines around communicating in a timely +fashion. + +We don't ask everyone to make this commitment. In particular, we don't believe +it's fair to expect quick responses from unpaid contributors, although we +gratefully accept any review work they do have time to contribute. + +If you are in the auto-assignment pool, remember: **You are not necessarily +expected to review the pull requests which are assigned to you.** Your only +responsibility is to ensure that contributors know what to expect from us, and +to arrange that _somebody_ reviews each PR. + +We have several different teams that reviewers may be auto-assigned from. You +should be in teams where you are likely to know who to re-assign a PR to, if you +can't review it yourself. The teams are determined by the `CODEOWNERS` file at +the root of the Wasmtime repository. But despite the name, membership in these +teams is _not_ about who is an authority or "owner" in a particular area. So +rather than creating a team for each fine-grained division in the repository +such as individual target architectures or WASI extensions, we use a few +coarse-grained teams: + +- [wasmtime-compiler-reviewers][]: Cranelift and Winch +- [wasmtime-core-reviewers][]: Wasmtime, including WASI +- [wasmtime-fuzz-reviewers][]: Fuzz testing targets +- [wasmtime-default-reviewers][]: Anything else, including CI and documentation + +[wasmtime-compiler-reviewers]: https://github.com/orgs/bytecodealliance/teams/wasmtime-compiler-reviewers +[wasmtime-core-reviewers]: https://github.com/orgs/bytecodealliance/teams/wasmtime-core-reviewers +[wasmtime-fuzz-reviewers]: https://github.com/orgs/bytecodealliance/teams/wasmtime-fuzz-reviewers +[wasmtime-default-reviewers]: https://github.com/orgs/bytecodealliance/teams/wasmtime-default-reviewers + +Ideally, auto-assigned reviewers should be attending the regular Wasmtime or +Cranelift meetings, as appropriate for the areas they're reviewing. This is to +help these reviewers stay aware of who is working on what, to more easily hand +off PRs to the most relevant reviewer for the work. However, this is only +advice, not a hard requirement. + +If you are not sure who to hand off a PR review to, you can look at GitHub's +suggestions for reviewers, or look at `git log` for the paths that the PR +affects. You can also just ask other Core Team members for advice. + +## General advice + +This is a collection of general advice for people who are reviewing pull +requests. Feel free to take any that you find works for you and ignore the rest. +You can also open pull requests to suggest more references for this section. + +[The Gentle Art of Patch Review][gentle-review] suggests a "Three-Phase +Contribution Review" process: + +[gentle-review]: https://sage.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/ + +1. Is the idea behind the contribution sound? +2. Is the contribution architected correctly? +3. Is the contribution polished? + +Phase one should be a quick check for whether the pull request should move +forward at all, or needs a significantly different approach. If it needs +significant changes or is not going to be accepted, there's no point reviewing +in detail until those issues are addressed. + +On the other end, it's a good idea to defer reviewing for typos or bikeshedding +about variable names until phase three. If there need to be significant +structural changes, entire paragraphs or functions might disappear, and then any +minor errors that were in them won't matter. + +The full essay has much more advice and is recommended reading. diff --git a/docs/contributing-maintainer-guidelines.md b/docs/contributing-maintainer-guidelines.md new file mode 100644 index 000000000000..81b420f2e272 --- /dev/null +++ b/docs/contributing-maintainer-guidelines.md @@ -0,0 +1,5 @@ +# Maintainer Guidelines + +This section describes procedures and expectations for Core Team members. It may +be of interest if you just want to understand how we work, or if you are joining +the Core Team yourself.