Revert "presubmit: Skip owners checks if code-owners plugin is enabled."

This reverts commit 2cf835a9ba.

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 <ehmaldonado@chromium.org>
> Reviewed-by: Gavin Mak <gavinmak@google.com>
> Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>

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 <sokcevic@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>

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."""

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)])

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

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):

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)

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='')