From ec3c39536aa728bd9b45e1095f5d01a686438802 Mon Sep 17 00:00:00 2001 From: Josip Sokcevic Date: Tue, 2 Mar 2021 22:30:31 +0000 Subject: [PATCH] Revert "presubmit: Skip owners checks if code-owners plugin is enabled." This reverts commit 2cf835a9bad9ef5bfe477719d3376eccab3cac68. Reason for revert: doesn't work with depot_tools error 404 Not found: tools (with applied patch from 2729404) Bug: 1183975 Original change's description: > presubmit: Skip owners checks if code-owners plugin is enabled. > > If code-owners plugin is enable for the repo, skip owners check on > commit, and skip checking owners format, as that will be done by > the plugin. > > Change-Id: I1663baef4f0f27b00423071343fe740f6da50ce7 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2727131 > Auto-Submit: Edward Lesmes > Reviewed-by: Gavin Mak > Commit-Queue: Edward Lesmes Change-Id: Id4d560701e4b0144e0aafc5263e09ed6927f6222 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2729409 Auto-Submit: Josip Sokcevic Commit-Queue: Rubber Stamper Bot-Commit: Rubber Stamper --- gerrit_util.py | 9 +-------- presubmit_canned_checks.py | 15 ++------------- presubmit_support.py | 7 ------- tests/git_cl_test.py | 9 ++++----- tests/owners_client_test.py | 4 ++-- tests/presubmit_unittest.py | 26 +++++++------------------- 6 files changed, 16 insertions(+), 54 deletions(-) diff --git a/gerrit_util.py b/gerrit_util.py index 1b6bca6ad..7704e2387 100644 --- a/gerrit_util.py +++ b/gerrit_util.py @@ -771,20 +771,13 @@ def SetCommitMessage(host, change, description, notify='ALL'): 'in change %s' % change) -def IsCodeOwnersEnabledOnHost(host): +def IsCodeOwnersEnabled(host): """Check if the code-owners plugin is enabled for the host.""" path = 'config/server/capabilities' capabilities = ReadHttpJsonResponse(CreateHttpConn(host, path)) return 'code-owners-checkCodeOwner' in capabilities -def IsCodeOwnersEnabledOnRepo(host, repo): - """Check if the code-owners plugin is enabled for the repo.""" - path = '/projects/%s/code_owners.project_config' % repo - config = ReadHttpJsonResponse(CreateHttpConn(host, path)) - return config['status'].get('disabled', False) - - def GetOwnersForFile(host, project, branch, path, limit=100, resolve_all_users=True, o_params=('DETAILS',)): """Gets information about owners attached to a file.""" diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 8c694d0e8..16c149765 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -1108,8 +1108,6 @@ def CheckOwnersDirMetadataExclusive(input_api, output_api): def CheckOwnersFormat(input_api, output_api): - if input_api.gerrit and input_api.gerrit.IsCodeOwnersEnabledOnRepo(): - return [] affected_files = set([ f.LocalPath() for f in input_api.change.AffectedFiles() @@ -1141,17 +1139,8 @@ def CheckOwners( if input_api.gerrit.IsOwnersOverrideApproved(input_api.change.issue): return [] - code_owners_enabled = False - if input_api.gerrit and input_api.gerrit.IsCodeOwnersEnabledOnRepo(): - code_owners_enabled = True - - # Skip OWNERS check on commit if code-owners plugin is enabled, as owners - # enforcement is done by the plugin. - if input_api.is_committing and code_owners_enabled: - return [] - - # Ignore tbr if the code-owners plugin is enabled on this repo. - tbr = not code_owners_enabled and input_api.tbr + # Ignore tbr if not allowed for this repo. + tbr = input_api.tbr and allow_tbr affected_files = set([f.LocalPath() for f in input_api.change.AffectedFiles(file_filter=source_file_filter)]) diff --git a/presubmit_support.py b/presubmit_support.py index 715a3baf7..3a05c4b8e 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -381,7 +381,6 @@ class GerritAccessor(object): self.project = project self.branch = branch self.cache = {} - self.code_owners_enabled = None def _FetchChangeDetail(self, issue): # Separate function to be easily mocked in tests. @@ -467,12 +466,6 @@ class GerritAccessor(object): def UpdateDescription(self, description, issue): gerrit_util.SetCommitMessage(self.host, issue, description, notify='NONE') - def IsCodeOwnersEnabledOnRepo(self): - if self.code_owners_enabled is None: - self.code_owners_enabled = gerrit_util.IsCodeOwnersEnabledOnRepo( - self.host, self.repo) - return self.code_owners_enabled - class OutputApi(object): """An instance of OutputApi gets passed to presubmit scripts so that they diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 3e70e9320..be35fc7c4 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -1505,7 +1505,7 @@ class TestGitCl(unittest.TestCase): change_id = git_cl.GenerateGerritChangeId('line1\nline2\n') self.assertEqual(change_id, 'Ihashchange') - @mock.patch('gerrit_util.IsCodeOwnersEnabledOnHost') + @mock.patch('gerrit_util.IsCodeOwnersEnabled') @mock.patch('git_cl.Settings.GetBugPrefix') @mock.patch('git_cl.Changelist.FetchDescription') @mock.patch('git_cl.Changelist.GetBranch') @@ -1517,7 +1517,7 @@ class TestGitCl(unittest.TestCase): self, mockBatchListOwners=None, mockGetRemoteBranch=None, mockGetGerritProject=None, mockGetGerritHost=None, mockGetBranch=None, mockFetchDescription=None, mockGetBugPrefix=None, - mockIsCodeOwnersEnabledOnHost=None, + mockIsCodeOwnersEnabled=None, initial_description='desc', bug=None, fixed=None, branch='branch', reviewers=None, tbrs=None, add_owners_to=None, expected_description='desc'): @@ -1528,7 +1528,7 @@ class TestGitCl(unittest.TestCase): 'b': ['b@example.com'], 'c': ['c@example.com'], } - mockIsCodeOwnersEnabledOnHost.return_value = True + mockIsCodeOwnersEnabled.return_value = True mockGetBranch.return_value = branch mockGetBugPrefix.return_value = 'prefix' mockGetRemoteBranch.return_value = ('origin', 'refs/remotes/origin/main') @@ -4126,8 +4126,7 @@ class CMDOwnersTestCase(CMDTestCaseBase): mock.patch( 'owners_client.OwnersClient.BatchListOwners', return_value=self.owners_by_path).start() - mock.patch( - 'gerrit_util.IsCodeOwnersEnabledOnHost', return_value=True).start() + mock.patch('gerrit_util.IsCodeOwnersEnabled', return_value=True).start() self.addCleanup(mock.patch.stopall) def testShowAllNoArgs(self): diff --git a/tests/owners_client_test.py b/tests/owners_client_test.py index 15336da52..5039ca735 100644 --- a/tests/owners_client_test.py +++ b/tests/owners_client_test.py @@ -319,7 +319,7 @@ class OwnersClientTest(unittest.TestCase): class GetCodeOwnersClientTest(unittest.TestCase): def setUp(self): - mock.patch('gerrit_util.IsCodeOwnersEnabledOnHost').start() + mock.patch('gerrit_util.IsCodeOwnersEnabled').start() self.addCleanup(mock.patch.stopall) def testGetCodeOwnersClient_GerritClient(self): @@ -328,7 +328,7 @@ class GetCodeOwnersClientTest(unittest.TestCase): pass def testGetCodeOwnersClient_DepotToolsClient(self): - gerrit_util.IsCodeOwnersEnabledOnHost.return_value = False + gerrit_util.IsCodeOwnersEnabled.return_value = False self.assertIsInstance( owners_client.GetCodeOwnersClient('root', 'host', 'project', 'branch'), owners_client.DepotToolsClient) diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index c4c2f9225..fc4e1df40 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -2668,14 +2668,13 @@ the current line as well! self.assertEqual(1, len(results)) self.assertIsInstance(results[0], presubmit.OutputApi.PresubmitError) - def GetInputApiWithOWNERS(self, owners_content, code_owners_enabled=False): + def GetInputApiWithOWNERS(self, owners_content): input_api = self.GetInputApiWithFiles({'OWNERS': ('M', owners_content)}) owners_file = StringIO(owners_content) fopen = lambda *args: owners_file input_api.owners_db = owners.Database('', fopen, os.path) - input_api.gerrit.IsCodeOwnersEnabledOnRepo = lambda: code_owners_enabled return input_api @@ -2690,17 +2689,6 @@ the current line as well! input_api, presubmit.OutputApi) ) - def testCheckOwnersFormatWorks_CodeOwners(self): - # If code owners is enabled, we rely on it to check owners format instead of - # depot tools. - input_api = self.GetInputApiWithOWNERS( - 'any content', code_owners_enabled=True) - self.assertEqual( - [], - presubmit_canned_checks.CheckOwnersFormat( - input_api, presubmit.OutputApi) - ) - def testCheckOwnersFormatFails(self): input_api = self.GetInputApiWithOWNERS('\n'.join([ 'set noparent', @@ -2716,7 +2704,7 @@ the current line as well! self, tbr=False, issue='1', approvers=None, modified_files=None, owners_by_path=None, is_committing=True, response=None, expected_output='', manually_specified_reviewers=None, dry_run=None, - code_owners_enabled=False): + allow_tbr=True): # The set of people who lgtm'ed a change. approvers = approvers or set() manually_specified_reviewers = manually_specified_reviewers or [] @@ -2760,14 +2748,13 @@ the current line as well! input_api.tbr = tbr input_api.dry_run = dry_run input_api.gerrit._FetchChangeDetail = lambda _: response - input_api.gerrit.IsCodeOwnersEnabledOnRepo = lambda: code_owners_enabled input_api.owners_client = owners_client.DepotToolsClient('root', 'branch') with mock.patch('owners_client.DepotToolsClient.ListOwners', side_effect=lambda f: owners_by_path.get(f, [])): results = presubmit_canned_checks.CheckOwners( - input_api, presubmit.OutputApi) + input_api, presubmit.OutputApi, allow_tbr=allow_tbr) for result in results: result.handle() if expected_output: @@ -3080,11 +3067,12 @@ the current line as well! def testCannedCheckOwners_TBRIgnored(self): self.AssertOwnersWorks( tbr=True, - code_owners_enabled=True, - expected_output='') + allow_tbr=False, + expected_output='Missing LGTM from someone other than ' + 'john@example.com\n') self.AssertOwnersWorks( tbr=True, - code_owners_enabled=True, + allow_tbr=False, is_committing=False, expected_output='')