From e4bab45474062a0108955b1020c81f1273fa1d16 Mon Sep 17 00:00:00 2001 From: Dirk Pranke Date: Fri, 19 Mar 2021 02:41:02 +0000 Subject: [PATCH] Reland "presubmit: Don't skip OWNERS check when Bot-Commit+1 is present." This reverts commit e7c058174054c97f05a1c4b009f5226bee5983e5. Reason for revert: Whoops. chromium/src was updated. I was confused by a conversation on the #ops slack channel and my checkout being slightly out of date. I think the original change was actually okay. Original change's description: > Revert "presubmit: Don't skip OWNERS check when Bot-Commit+1 is present." > > This reverts commit 9757ad58837498ee2d8e4a33deb2c91af19d6cf1. > > Reason for revert: We need to update callers first (e.g., //PRESUBMIT.py in chromium/src). > > Original change's description: > > presubmit: Don't skip OWNERS check when Bot-Commit+1 is present. > > > > Change-Id: I17b07796a86c5214e13a0058428889c1bb2b850d > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2774080 > > Auto-Submit: Edward Lesmes > > Reviewed-by: Jason Clinton > > Commit-Queue: Edward Lesmes > > Change-Id: I02c3d5ea2e65ef852d34a6816d04fe1cad82823a > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2775101 > Auto-Submit: Dirk Pranke > Commit-Queue: Rubber Stamper > Bot-Commit: Rubber Stamper Change-Id: I2c1ae8c60938cbd9316e9075488cc7451068a2f2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2775103 Bot-Commit: Rubber Stamper Reviewed-by: Dirk Pranke Reviewed-by: Jason Clinton Auto-Submit: Dirk Pranke Commit-Queue: Dirk Pranke --- presubmit_canned_checks.py | 17 ++++++----------- presubmit_support.py | 3 --- tests/presubmit_unittest.py | 31 ------------------------------- 3 files changed, 6 insertions(+), 45 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 16c149765..d4714e5d4 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -1127,17 +1127,12 @@ def CheckOwnersFormat(input_api, output_api): def CheckOwners( input_api, output_api, source_file_filter=None, allow_tbr=True): - if input_api.change.issue: - # Skip OWNERS check when Bot-Commit label is approved. This label is - # intended for commits made by trusted bots that don't require review nor - # owners approval. - if input_api.gerrit.IsBotCommitApproved(input_api.change.issue): - return [] - # Skip OWNERS check when Owners-Override label is approved. This is intended - # for global owners, trusted bots, and on-call sheriffs. Review is still - # required for these changes. - if input_api.gerrit.IsOwnersOverrideApproved(input_api.change.issue): - return [] + # Skip OWNERS check when Owners-Override label is approved. This is intended + # for global owners, trusted bots, and on-call sheriffs. Review is still + # required for these changes. + if (input_api.change.issue + and input_api.gerrit.IsOwnersOverrideApproved(input_api.change.issue)): + return [] # Ignore tbr if not allowed for this repo. tbr = input_api.tbr and allow_tbr diff --git a/presubmit_support.py b/presubmit_support.py index 3a05c4b8e..c53ed8fa1 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -446,9 +446,6 @@ class GerritAccessor(object): return [v for v in label_info.get('all', []) if v.get('value', 0) == max_value] - def IsBotCommitApproved(self, issue): - return bool(self._GetApproversForLabel(issue, 'Bot-Commit')) - def IsOwnersOverrideApproved(self, issue): return bool(self._GetApproversForLabel(issue, 'Owners-Override')) diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index fc4e1df40..739a2e339 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -2795,37 +2795,6 @@ the current line as well! response=response, expected_output='') - def testCannedCheckOwners_BotCommit(self): - response = { - "owner": {"email": "john@example.com"}, - "labels": {"Bot-Commit": { - u'all': [ - { - u'email': u'bot@example.com', - u'value': 1 - }, - ], - u'approved': {u'email': u'bot@example.org'}, - u'default_value': 0, - u'values': {u' 0': u'No score', - u'+1': u'Looks good to me'}, - }}, - "reviewers": {"REVIEWER": [{u'email': u'bot@example.com'}]}, - } - self.AssertOwnersWorks( - approvers=set(), - modified_files={'foo/xyz.cc': ['john@example.com']}, - response=response, - is_committing=True, - expected_output='') - - self.AssertOwnersWorks( - approvers=set(), - modified_files={'foo/xyz.cc': ['john@example.com']}, - is_committing=False, - response=response, - expected_output='') - def testCannedCheckOwners_OwnersOverride(self): response = { "owner": {"email": "john@example.com"},