Use GetCodeOwnersClient in presubmit_support
This change also adds a target_ref flag to presubmit_support.py.
Recipe-Nontrivial-Roll: build
Change-Id: I6de6bb87fc1482b88d9fbebe5e4ad1dbd8ce9748
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2702792
Commit-Queue: Gavin Mak <gavinmak@google.com>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
diff --git a/git_cl.py b/git_cl.py
index 141364b..d26302f 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -1294,6 +1294,8 @@
gerrit_url = self.GetCodereviewServer()
issue = self.GetIssue()
patchset = self.GetPatchset()
+ remote, remote_branch = self.GetRemoteBranch()
+ target_ref = GetTargetRef(remote, remote_branch, None)
if author:
args.extend(['--author', author])
if gerrit_url:
@@ -1302,6 +1304,8 @@
args.extend(['--issue', str(issue)])
if patchset:
args.extend(['--patchset', str(patchset)])
+ if target_ref:
+ args.extend(['--target_ref', target_ref])
return args
diff --git a/owners_client.py b/owners_client.py
index 14c1bf5..915412e 100644
--- a/owners_client.py
+++ b/owners_client.py
@@ -228,6 +228,6 @@
Defaults to GerritClient, and falls back to DepotToolsClient if code-owners
plugin is not available."""
- if gerrit_util.IsCodeOwnersEnabled(host):
+ if host and gerrit_util.IsCodeOwnersEnabled(host):
return GerritClient(host, project, branch)
return DepotToolsClient(root, branch)
diff --git a/presubmit_support.py b/presubmit_support.py
index 7b03962..70b3489 100755
--- a/presubmit_support.py
+++ b/presubmit_support.py
@@ -376,8 +376,10 @@
To avoid excessive Gerrit calls, caches the results.
"""
- def __init__(self, host):
+ def __init__(self, host, project=None, branch=None):
self.host = host
+ self.project = project
+ self.branch = branch
self.cache = {}
def _FetchChangeDetail(self, issue):
@@ -650,10 +652,19 @@
# Temporary files we must manually remove at the end of a run.
self._named_temporary_files = []
- # TODO(dpranke): figure out a list of all approved owners for a repo
- # in order to be able to handle wildcard OWNERS files?
- self.owners_client = owners_client.DepotToolsClient(
- change.RepositoryRoot(), change.UpstreamBranch(), os_path=self.os_path)
+ host = ''
+ project = ''
+ branch = ''
+ if gerrit_obj:
+ host = gerrit_obj.host or ''
+ project = gerrit_obj.project or ''
+ branch = gerrit_obj.branch or ''
+
+ self.owners_client = owners_client.GetCodeOwnersClient(
+ root=change.RepositoryRoot(),
+ host=host,
+ project=project,
+ branch=branch)
self.owners_db = owners_db.Database(
change.RepositoryRoot(), fopen=open, os_path=self.os_path)
self.owners_finder = owners_finder.OwnersFinder
@@ -1507,7 +1518,7 @@
class PresubmitExecuter(object):
def __init__(self, change, committing, verbose, gerrit_obj, dry_run=None,
- thread_pool=None, parallel=False, gerrit_project=None):
+ thread_pool=None, parallel=False):
"""
Args:
change: The Change object.
@@ -1525,7 +1536,6 @@
self.more_cc = []
self.thread_pool = thread_pool
self.parallel = parallel
- self.gerrit_project = gerrit_project
def ExecPresubmitScript(self, script_text, presubmit_path):
"""Executes a single presubmit script.
@@ -1568,9 +1578,10 @@
# Get the URL of git remote origin and use it to identify host and project
host = ''
- if self.gerrit and self.gerrit.host:
- host = self.gerrit.host
- project = self.gerrit_project or ''
+ project = ''
+ if self.gerrit:
+ host = self.gerrit.host or ''
+ project = self.gerrit.project or ''
# Prefix for test names
prefix = 'presubmit:%s/%s:%s/' % (host, project, rel_path)
@@ -1669,8 +1680,7 @@
gerrit_obj,
dry_run=None,
parallel=False,
- json_output=None,
- gerrit_project=None):
+ json_output=None):
"""Runs all presubmit checks that apply to the files in the change.
This finds all PRESUBMIT.py files in directories enclosing the files in the
@@ -1713,7 +1723,7 @@
results = []
thread_pool = ThreadPool()
executer = PresubmitExecuter(change, committing, verbose, gerrit_obj,
- dry_run, thread_pool, parallel, gerrit_project)
+ dry_run, thread_pool, parallel)
if default_presubmit:
if verbose:
sys.stdout.write('Running default presubmit script.\n')
@@ -1870,11 +1880,15 @@
parser: The parser used to parse the arguments from command line.
options: The arguments parsed from command line.
Returns:
- A GerritAccessor object if options.gerrit_url is set, or None otherwise.
+ A GerritAccessor object if options.gerrit_url or options.gerrit_project are
+ set, or None otherwise.
"""
gerrit_obj = None
- if options.gerrit_url:
- gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc)
+ if options.gerrit_url or options.gerrit_project:
+ gerrit_obj = GerritAccessor(
+ urlparse.urlparse(options.gerrit_url or '').netloc,
+ options.gerrit_project,
+ options.target_ref)
if not options.gerrit_fetch:
return gerrit_obj
@@ -1960,6 +1974,8 @@
'executing presubmit or post-upload hooks. fnmatch '
'wildcards can also be used.')
parser.add_argument('--gerrit_project', help=argparse.SUPPRESS)
+ parser.add_argument('--target_ref',
+ help='The remote branch ref to use for the change.')
options = parser.parse_args(argv)
log_level = logging.ERROR
@@ -1992,8 +2008,7 @@
gerrit_obj,
options.dry_run,
options.parallel,
- options.json_output,
- options.gerrit_project)
+ options.json_output)
except PresubmitFailure as e:
print(e, file=sys.stderr)
print('Maybe your depot_tools is out of date?', file=sys.stderr)
diff --git a/recipes/recipe_modules/presubmit/api.py b/recipes/recipe_modules/presubmit/api.py
index 3fbdd2d..0093a25 100644
--- a/recipes/recipe_modules/presubmit/api.py
+++ b/recipes/recipe_modules/presubmit/api.py
@@ -98,6 +98,7 @@
'--issue', self.m.tryserver.gerrit_change.change,
'--patchset', self.m.tryserver.gerrit_change.patchset,
'--gerrit_url', 'https://%s' % self.m.tryserver.gerrit_change.host,
+ '--target_ref', self.m.tryserver.gerrit_change_target_ref,
'--gerrit_fetch',
]
if self.m.cq.state == self.m.cq.DRY:
diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py
index fdc884c..730084e 100755
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@@ -2958,6 +2958,9 @@
mock.patch('git_cl.Changelist.GetAuthor', return_value='author').start()
mock.patch('git_cl.Changelist.GetIssue', return_value=123456).start()
mock.patch('git_cl.Changelist.GetPatchset', return_value=7).start()
+ mock.patch('git_cl.Changelist.GetRemoteBranch',
+ return_value=('foo', 'bar')).start()
+ mock.patch('git_cl.GetTargetRef', return_value='ref').start()
mock.patch('git_cl.PRESUBMIT_SUPPORT', 'PRESUBMIT_SUPPORT').start()
mock.patch('git_cl.Settings.GetRoot', return_value='root').start()
mock.patch('git_cl.time_time').start()
@@ -3000,6 +3003,7 @@
'--gerrit_url', 'https://chromium-review.googlesource.com',
'--issue', '123456',
'--patchset', '7',
+ '--target_ref', 'ref',
'--commit',
'--may_prompt',
'--parallel',
@@ -3048,6 +3052,7 @@
'vpython', 'PRESUBMIT_SUPPORT',
'--root', 'root',
'--upstream', 'upstream',
+ '--target_ref', 'ref',
'--upload',
'--json_output', '/tmp/fake-temp2',
'--description_file', '/tmp/fake-temp1',
@@ -3095,6 +3100,7 @@
'vpython', 'PRESUBMIT_SUPPORT',
'--root', 'root',
'--upstream', 'upstream',
+ '--target_ref', 'ref',
'--upload',
'--json_output', '/tmp/fake-temp2',
'--description_file', '/tmp/fake-temp1',
@@ -3135,6 +3141,7 @@
'--gerrit_url', 'https://chromium-review.googlesource.com',
'--issue', '123456',
'--patchset', '7',
+ '--target_ref', 'ref',
'--post_upload',
'--description_file', '/tmp/fake-temp1',
])
diff --git a/tests/owners_client_test.py b/tests/owners_client_test.py
index 7695f9c..6f6369f 100644
--- a/tests/owners_client_test.py
+++ b/tests/owners_client_test.py
@@ -275,6 +275,13 @@
mock.patch('gerrit_util.IsCodeOwnersEnabled').start()
self.addCleanup(mock.patch.stopall)
+ def testGetCodeOwnersClient_NoHost(self):
+ owners_client.GetCodeOwnersClient('root', None, 'project', 'branch')
+ gerrit_util.IsCodeOwnersEnabled.assert_not_called()
+
+ owners_client.GetCodeOwnersClient('root', '', 'project', 'branch')
+ gerrit_util.IsCodeOwnersEnabled.assert_not_called()
+
def testGetCodeOwnersClient_GerritClient(self):
gerrit_util.IsCodeOwnersEnabled.return_value = True
self.assertIsInstance(
diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py
index d745c7c..dad2c4e 100755
--- a/tests/presubmit_unittest.py
+++ b/tests/presubmit_unittest.py
@@ -184,6 +184,7 @@
mock.patch('os.path.abspath', lambda f: f).start()
mock.patch('os.path.isfile').start()
mock.patch('os.remove').start()
+ mock.patch('owners_client.GetCodeOwnersClient').start()
mock.patch('presubmit_support._parse_files').start()
mock.patch('presubmit_support.rdb_wrapper.client',
return_value=self.rdb_client).start()
@@ -1144,9 +1145,10 @@
upstream=options.upstream)
scm.GIT.GetAllFiles.assert_called_once_with(options.root)
- def testParseGerritOptions_NoGerritUrl(self):
+ def testParseGerritOptions_NoGerritUrlOrProject(self):
options = mock.Mock(
gerrit_url=None,
+ gerrit_project=None,
gerrit_fetch=False,
author='author',
description='description')
@@ -1160,14 +1162,16 @@
def testParseGerritOptions_NoGerritFetch(self):
options = mock.Mock(
gerrit_url='https://foo-review.googlesource.com/bar',
+ gerrit_project='baz/project',
gerrit_fetch=False,
+ target_ref='refs/heads/foobar',
author='author',
description='description')
gerrit_obj = presubmit._parse_gerrit_options(None, options)
presubmit.GerritAccessor.assert_called_once_with(
- 'foo-review.googlesource.com')
+ 'foo-review.googlesource.com', 'baz/project', 'refs/heads/foobar')
self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj)
self.assertEqual('author', options.author)
self.assertEqual('description', options.description)
@@ -1181,14 +1185,16 @@
options = mock.Mock(
gerrit_url='https://foo-review.googlesource.com/bar',
+ gerrit_project='baz/project',
gerrit_fetch=True,
+ target_ref='refs/heads/foobar',
issue=123,
patchset=4)
gerrit_obj = presubmit._parse_gerrit_options(None, options)
presubmit.GerritAccessor.assert_called_once_with(
- 'foo-review.googlesource.com')
+ 'foo-review.googlesource.com', 'baz/project', 'refs/heads/foobar')
self.assertEqual(presubmit.GerritAccessor.return_value, gerrit_obj)
self.assertEqual('new owner', options.author)
self.assertEqual('new description', options.description)
