Skip to content

Commit

Permalink
Update documentation to allow owners of public APIs that are used thr…
Browse files Browse the repository at this point in the history 
…oughout the codebase to Owners-Override changes across the source tree.

Unlike LSC which is meant for work that spans dozens of CLs, this new change is meant for one-off CLs. It's similar to google3's global approvals mechanism.

base, build, blink, content and url were chosen as an initial set because they are used in many top level directories and are frequently changing.

Also add "set noparent" to top level directories to avoid inadvertent +1s from src/OWNERS bypass owners reviews sometimes.

Change-Id: Iaa397a5e09174139af21fee34dca390de132e216
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2688805
Auto-Submit: John Abd-El-Malek <[email protected]>
Commit-Queue: John Abd-El-Malek <[email protected]>
Reviewed-by: Takuto Ikuta <[email protected]>
Reviewed-by: Kentaro Hara <[email protected]>
Cr-Commit-Position: refs/heads/master@{#857391}
  • Loading branch information
John Abd-El-Malek authored and Chromium LUCI CQ committed Feb 24, 2021
1 parent 6ed1ce2 commit dfd1edc
Show file tree
Hide file tree
Showing 54 changed files with 110 additions and 14 deletions.
2 changes: 2 additions & 0 deletions ENG_REVIEW_OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Current list of eng reviewers mostly for the purpose of reviewing
# addings of top-level and third-party directories.

# NOTE: keep this in sync with [email protected] owners
[email protected]
[email protected]
[email protected]
Expand All @@ -9,3 +10,4 @@ [email protected]
[email protected]
[email protected]
[email protected]
# NOTE: keep this in sync with [email protected] owners
9 changes: 9 additions & 0 deletions OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
# OWNERS_STATUS = build/OWNERS.status

# Eng reviewer. Please reach out before adding new top-level directories.
# Note: this list is not for rubber-stamping mechanical changes that span the
# code base. Please reach out to owners of top-level directories instead.
file://ENG_REVIEW_OWNERS

# For global approvals. When an API changes in one of these directories the
# owner's +1 will therefore remove the need to get +1 for each other directory
# that was affected as a result.
file://base/OWNERS
file://content/OWNERS
file://third_party/blink/public/OWNERS

per-file [email protected]
per-file .clang-tidy=file://styleguide/c++/OWNERS
per-file .eslintrc.js=file://tools/web_dev_style/OWNERS
Expand Down
14 changes: 8 additions & 6 deletions PRESUBMIT.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3153,12 +3153,14 @@ def CheckSetNoParent(input_api, output_api):
found_owners_files.add(glob)

# Check that every set noparent line has a corresponding file:// line
# listed in build/OWNERS.setnoparent.
for set_noparent_line in found_set_noparent_lines:
if set_noparent_line in found_owners_files:
continue
errors.append(' %s:%d' % (f.LocalPath(),
found_set_noparent_lines[set_noparent_line]))
# listed in build/OWNERS.setnoparent. An exception is made for top level
# directories since src/OWNERS shouldn't review them.
if f.LocalPath().count('/') != 1:
for set_noparent_line in found_set_noparent_lines:
if set_noparent_line in found_owners_files:
continue
errors.append(' %s:%d' % (f.LocalPath(),
found_set_noparent_lines[set_noparent_line]))

results = []
if errors:
Expand Down
18 changes: 15 additions & 3 deletions PRESUBMIT_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3563,10 +3563,23 @@ def testErrorMissingHeader(self):


class SetNoParentTest(unittest.TestCase):
def testSetNoParentMissing(self):
def testSetNoParentTopLevelAllowed(self):
mock_input_api = MockInputApi()
mock_input_api.files = [
MockAffectedFile('goat/OWNERS',
[
'set noparent',
'[email protected]',
])
]
mock_output_api = MockOutputApi()
errors = PRESUBMIT.CheckSetNoParent(mock_input_api, mock_output_api)
self.assertEqual([], errors)

def testSetNoParentMissing(self):
mock_input_api = MockInputApi()
mock_input_api.files = [
MockAffectedFile('services/goat/OWNERS',
[
'set noparent',
'[email protected]',
Expand All @@ -3580,11 +3593,10 @@ def testSetNoParentMissing(self):
self.assertTrue('goat/OWNERS:1' in errors[0].long_text)
self.assertTrue('goat/OWNERS:3' in errors[0].long_text)


def testSetNoParentWithCorrectRule(self):
mock_input_api = MockInputApi()
mock_input_api.files = [
MockAffectedFile('goat/OWNERS',
MockAffectedFile('services/goat/OWNERS',
[
'set noparent',
'file://ipc/SECURITY_OWNERS',
Expand Down
1 change: 1 addition & 0 deletions android_webview/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions apps/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
# Apps team members
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions ash/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
3 changes: 3 additions & 0 deletions base/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
# See //base/README.md to find qualification for being an owner.

set noparent
# NOTE: keep this in sync with [email protected] owners
[email protected]
[email protected]
[email protected]
Expand All @@ -11,6 +13,7 @@ [email protected]
[email protected]
[email protected]
[email protected]
# NOTE: keep this in sync with [email protected] owners

# per-file rules:
# These are for the common case of adding or renaming files. If you're doing
Expand Down
3 changes: 3 additions & 0 deletions build/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
set noparent
# NOTE: keep this in sync with [email protected] owners
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
[email protected]
# NOTE: keep this in sync with [email protected] owners

# Clang build config changes:
[email protected]
Expand Down
1 change: 1 addition & 0 deletions build_overrides/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
file://build/OWNERS
[email protected]
[email protected]
1 change: 1 addition & 0 deletions buildtools/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
1 change: 1 addition & 0 deletions cc/OWNERS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
# Folks listed as unofficial can't do OWNERS approvals but are good people to
# ask for informal reviews.

set noparent
# layers
[email protected]
[email protected]
Expand Down
2 changes: 2 additions & 0 deletions chrome/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# This OWNERS list is a last resort. Please prefer to use more specific OWNERS
# where possible.

set noparent

# Reviewers (in CET):
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions chromecast/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
2 changes: 2 additions & 0 deletions chromeos/OWNERS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
# https://chromium.googlesource.com/chromiumos/chromite/+/master/scripts/chrome_chromeos_lkgm.py#38
# see also https://crbug.com/917623 and https://crbug.com/919552.

set noparent

per-file BUILD.gn=*
per-file CHROMEOS_LKGM=*
# Translation artifacts:
Expand Down
2 changes: 2 additions & 0 deletions cloud_print/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
set noparent

# Cloud Print is temporarily owned by the Chrome Print team.
file://printing/OWNERS

Expand Down
1 change: 1 addition & 0 deletions components/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
3 changes: 3 additions & 0 deletions content/OWNERS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
# For public questions directed to OWNERS, you can send email
# to [email protected].

set noparent
# NOTE: keep this in sync with [email protected] owners
[email protected]
[email protected]
[email protected]
Expand All @@ -24,6 +26,7 @@ [email protected]
[email protected]
[email protected]
[email protected]
# NOTE: keep this in sync with [email protected] owners

# per-file rules:
# These are for the common case of adding or renaming files. If you're doing
Expand Down
1 change: 1 addition & 0 deletions courgette/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions crypto/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
1 change: 1 addition & 0 deletions dbus/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
1 change: 1 addition & 0 deletions device/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]

Expand Down
16 changes: 12 additions & 4 deletions docs/code_reviews.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ touching. Any committer can review code, but an owner must provide a review
for each directory you are touching. If you have doubts, look at the git blame
for the file and the `OWNERS` files (see below).

To indicate a positive review, the reviewer provides a "Code-Review +1" in
To indicate a positive review, the reviewer provides a `Code-Review +1` in
Gerrit, also known as an LGTM ("Looks Good To Me"). A score of "-1" indicates
the change should not be submitted as-is.

Expand Down Expand Up @@ -168,15 +168,23 @@ per-file *_messages*.h=file://ipc/SECURITY_OWNERS

### Owners-Override

Setting the `Owners-Override` label will bypass OWNERS enforcement. Active
sheriffs and Large Scale Changes (see below) reviewers have this power.
Setting the `Owners-Override +1` label will bypass OWNERS enforcement. Active
sheriffs, Large Scale Changes and Global Approvers (see below) reviewers have
this capability.

## Mechanical changes

### Large Scale Changes
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.
whole codebase. This is used for work that span many dozen CLs.

### Global Approvals
For one-off CLs API owners of base, blink, build, content and url can
Owners-Override +1 a change to their APIs to avoid waiting for rubberstamp +1s
from affected directories' owners. This should only be used for mechanical
updates to the affected directories.

## Documentation updates

Expand Down
1 change: 1 addition & 0 deletions extensions/OWNERS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
# chrome/renderer/extensions/OWNERS
# chrome/renderer/resources/extensions/OWNERS

set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions fuchsia/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
file://build/fuchsia/OWNERS

per-file SECURITY_OWNERS=set noparent
Expand Down
1 change: 1 addition & 0 deletions gin/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
1 change: 1 addition & 0 deletions google_apis/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
file://components/signin/OWNERS

per-file BUILD.gn=*
1 change: 1 addition & 0 deletions google_update/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
set noparent
[email protected]
[email protected]
1 change: 1 addition & 0 deletions gpu/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions headless/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]

Expand Down
2 changes: 2 additions & 0 deletions infra/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# This is reserved for people that are comfortable with Chromium builders
# and understand the implications of changing their configurations.

set noparent

# Troopers
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions ios/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions ipc/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions jingle/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
set noparent
[email protected]
[email protected]
1 change: 1 addition & 0 deletions media/OWNERS
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
# and to load balance. Only use OWNERS in this file for these subdirectories
# when doing refactorings and general cleanups.

set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions mojo/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions native_client_sdk/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
1 change: 1 addition & 0 deletions net/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions pdf/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions ppapi/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
set noparent
[email protected]
[email protected]
1 change: 1 addition & 0 deletions printing/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions remoting/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions rlz/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
set noparent
[email protected]
[email protected]
1 change: 1 addition & 0 deletions sandbox/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions services/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
1 change: 1 addition & 0 deletions skia/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
2 changes: 2 additions & 0 deletions sql/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
set noparent

# Primary:
[email protected]

Expand Down
1 change: 1 addition & 0 deletions storage/OWNERS
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
set noparent
[email protected]
[email protected]
[email protected]
Expand Down
Loading

0 comments on commit dfd1edc

Please sign in to comment.