InTheForest/chromium_depot_tools
1
0
Fork 0
mirror of https://chromium.googlesource.com/chromium/tools/depot_tools.git synced 2026-01-11 18:51:29 +00:00
Code Issues Packages Projects Releases Wiki Activity

CheckNewDEPSHooksHasRequiredReviewers computes reviewers from OWNERS

Browse Source 
Instead of taking reviewers as input args, it computes the reviewers
from the top-level OWNERS file because otherwise, all callers will
basically do the same thing at the callsite.

Bug: 396736534
Change-Id: I3ea83b8285fab8b75cb77b40b10414c6a3dea6b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/6313624
Commit-Queue: Yiwei Zhang <yiwzhang@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
This commit is contained in:
Yiwei Zhang
2025-03-03 12:08:55 -08:00
committed by LUCI CQ
parent 85ec2718b5
commit 191c73e7a2
2 changed files with 43 additions and 20 deletions
Download Patch File Download Diff File Expand all files Collapse all files

41
presubmit_canned_checks.py
View File

@@ -2221,11 +2221,22 @@ def CheckNoNewGitFilesAddedInDependencies(input_api, output_api):
return errors
def CheckNewDEPSHooksHasRequiredReviewers(input_api, output_api,
required_reviewers: list[str]):
"""Ensure CL to add new DEPS hook(s) has at least one required reviewer."""
if not required_reviewers:
raise ValueError('required_reviewers must be non-empty')
def CheckNewDEPSHooksHasRequiredReviewers(input_api, output_api):
"""Ensures proper review of CLs adding new DEPS hooks.
This check verifies that any CL adding a new DEPS hook is reviewed and
approved by the required reviewer(s). If the required reviewers haven't
been added, an error will be raised.
Required Reviewers are computed from the OWNERS file. It looks for DEPS
owners annotated `For new DEPS hook`. Example:
`per-file DEPS=foo@chromium.org # For new DEPS hook`
This check is skipped if:
* The Gerrit CL hasn't been uploaded yet.
* The CL doesn't involve adding new hooks to the DEPS file.
* No required reviewers are found in the OWNERS file.
"""
if not input_api.change.issue:
return [] # Gerrit CL not yet uploaded.
deps_affected_files = [
@@ -2233,18 +2244,32 @@ def CheckNewDEPSHooksHasRequiredReviewers(input_api, output_api,
]
if not deps_affected_files:
return [] # Not a DEPS change.
deps_affected_file = deps_affected_files[0]
dep_file = deps_affected_files[0]
def _get_hooks_names(dep_contents):
deps = _ParseDeps('\n'.join(dep_contents))
hooks = deps.get('hooks', [])
return set(hook.get('name') for hook in hooks)
old_hooks = _get_hooks_names(deps_affected_file.OldContents())
new_hooks = _get_hooks_names(deps_affected_file.NewContents())
old_hooks = _get_hooks_names(dep_file.OldContents())
new_hooks = _get_hooks_names(dep_file.NewContents())
if new_hooks.issubset(old_hooks):
return [] # No new hooks added.
required_reviewer_re = input_api.re.compile(
r"^per-file\s+DEPS\s*=\s*({email_re}(\s*,\s*{email_re})*)\s*# For new DEPS hook$"
.format(email_re=r'[^ @]+@[^ #]+'))
required_reviewers = []
for line in input_api.ReadFile(
input_api.os_path.join(input_api.change.RepositoryRoot(),
'OWNERS')).splitlines():
if (m := required_reviewer_re.match(line)):
required_reviewers.extend(r.strip() for r in m.group(1).split(','))
if not required_reviewers:
return []
# TODO - crbug/396736534: return error if CL is not approved by the
# required reviewers during submission.
reviewers = input_api.gerrit.GetChangeReviewers(input_api.change.issue,
approving_only=False)

22
tests/presubmit_canned_checks_test.py
View File

@@ -539,16 +539,12 @@ class CheckNewDEPSHooksHasRequiredReviewersTest(unittest.TestCase):
def setUp(self):
self.input_api = MockInputApi()
self.input_api.change = MockChange([], issue=123)
self.input_api.files = [
MockAffectedFile('DEPS', 'content'),
]
self.input_api.change.RepositoryRoot = lambda: ''
def test_no_gerrit_cl(self):
self.input_api.change = MockChange([], issue=None)
results = presubmit_canned_checks.CheckNewDEPSHooksHasRequiredReviewers(
self.input_api,
MockOutputApi(),
required_reviewers=['foo@chromium.org'])
self.input_api, MockOutputApi())
self.assertEqual(0, len(results))
def test_no_deps_file_change(self):
@@ -556,9 +552,7 @@ class CheckNewDEPSHooksHasRequiredReviewersTest(unittest.TestCase):
MockAffectedFile('foo.py', 'content'),
]
results = presubmit_canned_checks.CheckNewDEPSHooksHasRequiredReviewers(
self.input_api,
MockOutputApi(),
required_reviewers=['foo@chromium.org'])
self.input_api, MockOutputApi())
self.assertEqual(0, len(results))
def test_new_deps_hook(self):
@@ -582,7 +576,7 @@ class CheckNewDEPSHooksHasRequiredReviewersTest(unittest.TestCase):
'expected_error_msg':
'New DEPS hooks (new_hook, new_hook_2) are found. Please add '
'one of the following reviewers:\n * foo@chromium.org\n '
'* bar@chromium.org'
'* bar@chromium.org\n * baz@chromium.org'
},
{
'name':
@@ -591,7 +585,7 @@ class CheckNewDEPSHooksHasRequiredReviewersTest(unittest.TestCase):
'new_contents': [
'hooks = [{"name": "old_hook"}, {"name": "new_hook"}, {"name": "new_hook_2"}]'
],
'reviewers': ['foo@chromium.org'],
'reviewers': ['baz@chromium.org'],
},
{
'name':
@@ -616,6 +610,10 @@ class CheckNewDEPSHooksHasRequiredReviewersTest(unittest.TestCase):
for case in test_cases:
with self.subTest(case_name=case['name']):
self.input_api.files = [
MockFile('OWNERS', [
'per-file DEPS=foo@chromium.org # For new DEPS hook',
'per-file DEPS=bar@chromium.org, baz@chromium.org # For new DEPS hook'
]),
MockAffectedFile('DEPS',
old_contents=case['old_contents'],
new_contents=case['new_contents']),
@@ -624,7 +622,7 @@ class CheckNewDEPSHooksHasRequiredReviewersTest(unittest.TestCase):
results = presubmit_canned_checks.CheckNewDEPSHooksHasRequiredReviewers(
self.input_api,
MockOutputApi(),
required_reviewers=['foo@chromium.org', 'bar@chromium.org'])
)
if 'expected_error_msg' in case:
self.assertEqual(1, len(results))
self.assertEqual(case['expected_error_msg'],