diff --git a/gerrit_util.py b/gerrit_util.py index 2fd436632..6923f8b9b 100644 --- a/gerrit_util.py +++ b/gerrit_util.py @@ -771,13 +771,21 @@ def SetCommitMessage(host, change, description, notify='ALL'): 'in change %s' % change) -def IsCodeOwnersEnabled(host): +def IsCodeOwnersEnabledOnHost(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.""" + repo = PercentEncodeForGitRef(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, seed=None, o_params=('DETAILS',)): """Gets information about owners attached to a file.""" diff --git a/owners_client.py b/owners_client.py index e70225c5b..27fa37a25 100644 --- a/owners_client.py +++ b/owners_client.py @@ -201,6 +201,6 @@ def GetCodeOwnersClient(root, upstream, host, project, branch): Defaults to GerritClient, and falls back to DepotToolsClient if code-owners plugin is not available.""" - if gerrit_util.IsCodeOwnersEnabled(host): + if gerrit_util.IsCodeOwnersEnabledOnHost(host): return GerritClient(host, project, branch) return DepotToolsClient(root, upstream) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index f76848db0..3e8b56b4d 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -1108,6 +1108,8 @@ 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() @@ -1134,8 +1136,17 @@ def CheckOwners( 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 + 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 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 9b339d4e4..a8ca57448 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -381,6 +381,7 @@ 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. @@ -466,6 +467,12 @@ 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 1a70abaee..539e53860 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.IsCodeOwnersEnabled') + @mock.patch('gerrit_util.IsCodeOwnersEnabledOnHost') @mock.patch('git_cl.Settings.GetBugPrefix') @mock.patch('git_cl.Changelist.FetchDescription') @mock.patch('git_cl.Changelist.GetBranch') @@ -1519,7 +1519,7 @@ class TestGitCl(unittest.TestCase): mockGetGerritProject=None, mockGetGerritHost=None, mockGetCommonAncestorWithUpstream=None, mockGetBranch=None, mockFetchDescription=None, mockGetBugPrefix=None, - mockIsCodeOwnersEnabled=None, + mockIsCodeOwnersEnabledOnHost=None, initial_description='desc', bug=None, fixed=None, branch='branch', reviewers=None, tbrs=None, add_owners_to=None, expected_description='desc'): @@ -1530,7 +1530,7 @@ class TestGitCl(unittest.TestCase): 'b': ['b@example.com'], 'c': ['c@example.com'], } - mockIsCodeOwnersEnabled.return_value = True + mockIsCodeOwnersEnabledOnHost.return_value = True mockGetBranch.return_value = branch mockGetBugPrefix.return_value = 'prefix' mockGetCommonAncestorWithUpstream.return_value = 'upstream' @@ -4139,7 +4139,8 @@ class CMDOwnersTestCase(CMDTestCaseBase): mock.patch( 'owners_client.OwnersClient.BatchListOwners', return_value=self.owners_by_path).start() - mock.patch('gerrit_util.IsCodeOwnersEnabled', return_value=True).start() + mock.patch( + 'gerrit_util.IsCodeOwnersEnabledOnHost', 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 701627e38..435be7d9f 100644 --- a/tests/owners_client_test.py +++ b/tests/owners_client_test.py @@ -292,17 +292,17 @@ class OwnersClientTest(unittest.TestCase): class GetCodeOwnersClientTest(unittest.TestCase): def setUp(self): - mock.patch('gerrit_util.IsCodeOwnersEnabled').start() + mock.patch('gerrit_util.IsCodeOwnersEnabledOnHost').start() self.addCleanup(mock.patch.stopall) def testGetCodeOwnersClient_GerritClient(self): - gerrit_util.IsCodeOwnersEnabled.return_value = True + gerrit_util.IsCodeOwnersEnabledOnHost.return_value = True self.assertIsInstance( owners_client.GetCodeOwnersClient('', '', 'host', 'project', 'branch'), owners_client.GerritClient) def testGetCodeOwnersClient_DepotToolsClient(self): - gerrit_util.IsCodeOwnersEnabled.return_value = False + gerrit_util.IsCodeOwnersEnabledOnHost.return_value = False self.assertIsInstance( owners_client.GetCodeOwnersClient('root', 'branch', '', '', ''), owners_client.DepotToolsClient) diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index fd929492f..8188bbb49 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -178,7 +178,7 @@ index fe3de7b..54ae6e1 100755 mock.patch('gclient_utils.FileWrite').start() mock.patch('json.load').start() mock.patch('multiprocessing.cpu_count', lambda: 2) - mock.patch('gerrit_util.IsCodeOwnersEnabled').start() + mock.patch('gerrit_util.IsCodeOwnersEnabledOnHost').start() mock.patch('os.chdir').start() mock.patch('os.getcwd', self.RootDir) mock.patch('os.listdir').start() @@ -2669,13 +2669,14 @@ the current line as well! self.assertEqual(1, len(results)) self.assertIsInstance(results[0], presubmit.OutputApi.PresubmitError) - def GetInputApiWithOWNERS(self, owners_content): + def GetInputApiWithOWNERS(self, owners_content, code_owners_enabled=False): 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,6 +2691,17 @@ 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', @@ -2705,7 +2717,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, - allow_tbr=True): + code_owners_enabled=False): # The set of people who lgtm'ed a change. approvers = approvers or set() manually_specified_reviewers = manually_specified_reviewers or [] @@ -2749,13 +2761,14 @@ 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.OwnersClient() with mock.patch('owners_client.OwnersClient.ListOwners', side_effect=lambda f: owners_by_path.get(f, [])): results = presubmit_canned_checks.CheckOwners( - input_api, presubmit.OutputApi, allow_tbr=allow_tbr) + input_api, presubmit.OutputApi) for result in results: result.handle() if expected_output: @@ -3037,12 +3050,11 @@ the current line as well! def testCannedCheckOwners_TBRIgnored(self): self.AssertOwnersWorks( tbr=True, - allow_tbr=False, - expected_output='Missing LGTM from someone other than ' - 'john@example.com\n') + code_owners_enabled=True, + expected_output='') self.AssertOwnersWorks( tbr=True, - allow_tbr=False, + code_owners_enabled=True, is_committing=False, expected_output='')