TBR has been removed; update docs
Change-Id: I610d35e92746b9ceeaff48198d176634466e51e8 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2684930 Auto-Submit: Jason Clinton <jclinton@chromium.org> Reviewed-by: Karan Bhatia <karandeepb@chromium.org> Commit-Queue: Karan Bhatia <karandeepb@chromium.org> Cr-Commit-Position: refs/heads/master@{#852337}
This commit is contained in:
Jason Clinton
committed by
Chromium LUCI CQ
Chromium LUCI CQ
parent
9f66d59ce3
commit
0daf7b01aa
@ -166,108 +166,35 @@ per-file *_messages*.h=set noparent
|
||||
per-file *_messages*.h=file://ipc/SECURITY_OWNERS
|
||||
```
|
||||
|
||||
## TBR ("To Be Reviewed")
|
||||
### Owners-Override
|
||||
|
||||
"TBR" is our mechanism for post-commit review. It should be used rarely and
|
||||
only in cases where a normal review is unnecessary, as described under
|
||||
"When to TBR", below.
|
||||
Setting the `Owners-Override` label will bypass OWNERS enforcement. Active
|
||||
sheriffs and Large Scale Changes (see below) reviewers have this power.
|
||||
|
||||
TBR does not mean "no review." A reviewer TBR-ed on a change should still
|
||||
review the change. If there are comments after landing, the author is obligated
|
||||
to address them in a followup patch.
|
||||
## Mechanical changes
|
||||
|
||||
Do not use TBR just because a change is urgent or the reviewer is being slow.
|
||||
Contact the reviewer directly or find somebody else to review your change.
|
||||
You can use the [Large Scale Changes](process/lsc/large_scale_changes.md)
|
||||
process to get approval to bypass OWNERS enforcement for large changes like
|
||||
refactoring, architectural changes, or other repetitive code changes across the
|
||||
whole codebase.
|
||||
|
||||
### How to TBR
|
||||
## Documentation updates
|
||||
|
||||
To send a change TBR, annotate the description and send email like normal.
|
||||
Otherwise the reviewer won't know to review the patch.
|
||||
Documentation updates require code review. We may revisit this decision in the
|
||||
future.
|
||||
|
||||
* Add the reviewer's email address in the code review tool's reviewer field
|
||||
like normal.
|
||||
## Automated code-review
|
||||
|
||||
* Add a line "Tbr: <reviewer's email>" to the bottom of the change list
|
||||
description. e.g. `Tbr: reviewer1@chromium.org,reviewer2@chromium.org`
|
||||
For verifiably safe changes like translation files, clean reverts, and clean
|
||||
cherry-picks, we have automation that will vote +1 on the `Bot-Commit` label
|
||||
allowing the CL to be submitted without human code-review. Add `Rubber Stamper`
|
||||
(rubber-stamper@appspot.gserviceaccount.com) to your CL as a reviewer to
|
||||
activate this automation. It will scan the CL after about 1 minute and reply
|
||||
with its verdict. `Bot-Commit` votes are not sticky between patchsets and so
|
||||
only add the bot once the CL is finalized.
|
||||
|
||||
* Type a message so that the owners in the TBR list can understand who is
|
||||
responsible for reviewing what, as part of their post-commit review
|
||||
responsibility. e.g.
|
||||
```
|
||||
TBRing reviewers:
|
||||
reviewer1: Please review changes to foo/
|
||||
reviewer2: Please review changes to bar/
|
||||
```
|
||||
When combined with the [`Owners-Override`](#owners_override) power discussed
|
||||
above, sheriffs can effectively revert and reland on their own.
|
||||
|
||||
### When to TBR
|
||||
|
||||
#### Reverts and relands
|
||||
|
||||
The most common use of TBR is to revert patches that broke the build. Clean
|
||||
reverts of recent patches may be submitted TBR. However, TBR should not be used
|
||||
if the revert required non-trivial conflict resolution, or if the patch being
|
||||
reverted is older than a few days.
|
||||
|
||||
A developer relanding a patch can TBR the OWNERS for changes which are identical
|
||||
to the original (reverted) patch. If the reland patch contains any new changes
|
||||
(such as bug fixes) on top of the original, those changes should go through the
|
||||
normal review process.
|
||||
|
||||
When creating a reland patch, you should first upload an up-to-date patchset
|
||||
with the exact content of the original (reverted) patch, and then upload the
|
||||
patchset to be relanded. This is important for the reviewers to understand what
|
||||
the fix for relanding was.
|
||||
|
||||
#### Mechanical changes
|
||||
|
||||
You can use TBR with certain mechanical changes that affect many callers in
|
||||
different directories. For example, adding a parameter to a common function in
|
||||
`//base`, with callers in `//chrome/browser/foo`, `//net/bar`, and many other
|
||||
directories. If the updates to the callers is mechanical, you can:
|
||||
|
||||
1. Get a normal owner of the lower-level code you're changing (in this
|
||||
example, the function in `//base`) to do a proper review of those changes.
|
||||
|
||||
2. Get _somebody_ to review the downstream changes made to the callers as a
|
||||
result of the `//base` change. This is often the same person from the
|
||||
previous step but could be somebody else.
|
||||
|
||||
3. TBR the owners of the calling code, after the API change is LGTM'ed.
|
||||
|
||||
This process ensures that all code is reviewed prior to checkin and that the
|
||||
concept of the change is reviewed by a qualified person, without having to ping
|
||||
many owners with little say in the trivial side-effects they incur.
|
||||
|
||||
**Note:** The above policy is only viable for strictly mechanical changes. For
|
||||
large-scale scripted changes you should:
|
||||
|
||||
1. Have an owner of the core change review the script.
|
||||
|
||||
2. Use `git cl split` to shard the large change into many small CLs with a
|
||||
clear description of what each reviewer is expected to verify
|
||||
([example](https://chromium-review.googlesource.com/1191225)).
|
||||
|
||||
#### Documentation updates
|
||||
|
||||
You can TBR documentation updates. Documentation means markdown files, text
|
||||
documents, and high-level comments in code. At finer levels of detail, comments
|
||||
in source files become more like code and should be reviewed normally (not
|
||||
using TBR). Non-TBR-able stuff includes things like function contracts and most
|
||||
comments inside functions.
|
||||
|
||||
* Use good judgement. If you're changing something very important, tricky,
|
||||
or something you may not be very familiar with, ask for the code review
|
||||
up-front.
|
||||
|
||||
* Don't TBR changes to policy documents like the style guide or this document.
|
||||
|
||||
* Don't mix unrelated documentation updates with code changes.
|
||||
|
||||
* Be sure to actually send out the email for the code review. If you get one,
|
||||
please actually read the changes.
|
||||
|
||||
## Automated code-review workflows
|
||||
|
||||
For verifiably safe changes like translation files, we have automation that
|
||||
will vote +1 on the `Bot-Commit` and `Owners-Override` labels, allowing the
|
||||
CL to be submitted without human code-review.
|
||||
Changes not supported by Rubber Stamper still need a +1 from another
|
||||
committer.
|
||||
|
||||
@ -20,8 +20,14 @@ necessary authority to fulfill them. In particular, you have the authority to:
|
||||
|
||||
* Revert changes that you know or suspect are causing breakages
|
||||
* Disable or otherwise mark misbehaving tests
|
||||
* Use TBRs freely as part of your sheriffing duties
|
||||
* Use Owners-Override label to override OWNERS checks freely as part of your
|
||||
sheriffing duties
|
||||
* Pull in any other engineer or team you need to help you do these duties
|
||||
* For clean reverts and cherry-picks, add the
|
||||
[Rubber Stamper bot](code_reviews.md#automated-code_review). All other
|
||||
changes require a +1 from another committer.
|
||||
|
||||
TBRs were removed in Q1 2021.
|
||||
|
||||
## How to be a Sheriff
|
||||
|
||||
|
||||
Reference in New Issue
Block a user