Commit e157691e authored by Edward Lesmes's avatar Edward Lesmes Committed by LUCI CQ

git-cl: Use code-owners plugin if available.

Change-Id: Ic88e6cac34c3a43f477b879cf7a5caa9014dfb49
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2692882
Auto-Submit: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: 's avatarJosip Sokcevic <sokcevic@google.com>
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
parent 0d1bdb23
...@@ -977,6 +977,7 @@ class Changelist(object): ...@@ -977,6 +977,7 @@ class Changelist(object):
# Lazily cached values. # Lazily cached values.
self._gerrit_host = None # e.g. chromium-review.googlesource.com self._gerrit_host = None # e.g. chromium-review.googlesource.com
self._gerrit_server = None # e.g. https://chromium-review.googlesource.com self._gerrit_server = None # e.g. https://chromium-review.googlesource.com
self._owners_client = None
# Map from change number (issue) to its detail cache. # Map from change number (issue) to its detail cache.
self._detail_cache = {} self._detail_cache = {}
...@@ -985,6 +986,18 @@ class Changelist(object): ...@@ -985,6 +986,18 @@ class Changelist(object):
self._gerrit_host = codereview_host self._gerrit_host = codereview_host
self._gerrit_server = 'https://%s' % codereview_host self._gerrit_server = 'https://%s' % codereview_host
@property
def owners_client(self):
if self._owners_client is None:
remote, remote_branch = self.GetRemoteBranch()
branch = GetTargetRef(remote, remote_branch, None)
self._owners_client = owners_client.GetCodeOwnersClient(
root=settings.GetRoot(),
host=self.GetGerritHost(),
project=self.GetGerritProject(),
branch=branch)
return self._owners_client
def GetCCList(self): def GetCCList(self):
"""Returns the users cc'd on this CL. """Returns the users cc'd on this CL.
...@@ -1375,16 +1388,14 @@ class Changelist(object): ...@@ -1375,16 +1388,14 @@ class Changelist(object):
# Fill gaps in OWNERS coverage to tbrs/reviewers if requested. # Fill gaps in OWNERS coverage to tbrs/reviewers if requested.
if options.add_owners_to: if options.add_owners_to:
assert options.add_owners_to in ('TBR', 'R'), options.add_owners_to assert options.add_owners_to in ('TBR', 'R'), options.add_owners_to
client = owners_client.DepotToolsClient( status = self.owners_client.GetFilesApprovalStatus(
root=settings.GetRoot(),
branch=self.GetCommonAncestorWithUpstream())
status = client.GetFilesApprovalStatus(
files, [], options.tbrs + options.reviewers) files, [], options.tbrs + options.reviewers)
missing_files = [ missing_files = [
f for f in files f for f in files
if status[f] == owners_client.OwnersClient.INSUFFICIENT_REVIEWERS if status[f] == self._owners_client.INSUFFICIENT_REVIEWERS
] ]
owners = client.SuggestOwners(missing_files, exclude=[self.GetAuthor()]) owners = self.owners_client.SuggestOwners(
missing_files, exclude=[self.GetAuthor()])
if options.add_owners_to == 'TBR': if options.add_owners_to == 'TBR':
assert isinstance(options.tbrs, list), options.tbrs assert isinstance(options.tbrs, list), options.tbrs
options.tbrs.extend(owners) options.tbrs.extend(owners)
...@@ -4790,10 +4801,7 @@ def CMDowners(parser, args): ...@@ -4790,10 +4801,7 @@ def CMDowners(parser, args):
if len(args) == 0: if len(args) == 0:
print('No files specified for --show-all. Nothing to do.') print('No files specified for --show-all. Nothing to do.')
return 0 return 0
client = owners_client.DepotToolsClient( owners_by_path = cl.owners_client.BatchListOwners(args)
root=settings.GetRoot(),
branch=cl.GetCommonAncestorWithUpstream())
owners_by_path = client.BatchListOwners(args)
for path in args: for path in args:
print('Owners for %s:' % path) print('Owners for %s:' % path)
print('\n'.join( print('\n'.join(
...@@ -4809,15 +4817,14 @@ def CMDowners(parser, args): ...@@ -4809,15 +4817,14 @@ def CMDowners(parser, args):
# Default to diffing against the common ancestor of the upstream branch. # Default to diffing against the common ancestor of the upstream branch.
base_branch = cl.GetCommonAncestorWithUpstream() base_branch = cl.GetCommonAncestorWithUpstream()
root = settings.GetRoot()
affected_files = cl.GetAffectedFiles(base_branch) affected_files = cl.GetAffectedFiles(base_branch)
if options.batch: if options.batch:
client = owners_client.DepotToolsClient(root, base_branch) owners = cl.owners_client.SuggestOwners(affected_files, exclude=[author])
print('\n'.join( print('\n'.join(owners))
client.SuggestOwners(affected_files, exclude=[cl.GetAuthor()])))
return 0 return 0
root = settings.GetRoot()
owner_files = [f for f in affected_files if 'OWNERS' in os.path.basename(f)] owner_files = [f for f in affected_files if 'OWNERS' in os.path.basename(f)]
original_owner_files = { original_owner_files = {
f: scm.GIT.GetOldContents(root, f, base_branch).splitlines() f: scm.GIT.GetOldContents(root, f, base_branch).splitlines()
......
...@@ -1505,16 +1505,19 @@ class TestGitCl(unittest.TestCase): ...@@ -1505,16 +1505,19 @@ class TestGitCl(unittest.TestCase):
change_id = git_cl.GenerateGerritChangeId('line1\nline2\n') change_id = git_cl.GenerateGerritChangeId('line1\nline2\n')
self.assertEqual(change_id, 'Ihashchange') self.assertEqual(change_id, 'Ihashchange')
@mock.patch('gerrit_util.IsCodeOwnersEnabled')
@mock.patch('git_cl.Settings.GetBugPrefix') @mock.patch('git_cl.Settings.GetBugPrefix')
@mock.patch('git_cl.Settings.GetRoot')
@mock.patch('git_cl.Changelist.FetchDescription') @mock.patch('git_cl.Changelist.FetchDescription')
@mock.patch('git_cl.Changelist.GetBranch') @mock.patch('git_cl.Changelist.GetBranch')
@mock.patch('git_cl.Changelist.GetCommonAncestorWithUpstream') @mock.patch('git_cl.Changelist.GetGerritHost')
@mock.patch('git_cl.Changelist.GetGerritProject')
@mock.patch('git_cl.Changelist.GetRemoteBranch')
@mock.patch('owners_client.OwnersClient.BatchListOwners') @mock.patch('owners_client.OwnersClient.BatchListOwners')
def getDescriptionForUploadTest( def getDescriptionForUploadTest(
self, mockBatchListOwners=None, mockGetCommonAncestorWithUpstream=None, self, mockBatchListOwners=None, mockGetRemoteBranch=None,
mockGetBranch=None, mockFetchDescription=None, mockGetRoot=None, mockGetGerritProject=None, mockGetGerritHost=None, mockGetBranch=None,
mockGetBugPrefix=None, mockFetchDescription=None, mockGetBugPrefix=None,
mockIsCodeOwnersEnabled=None,
initial_description='desc', bug=None, fixed=None, branch='branch', initial_description='desc', bug=None, fixed=None, branch='branch',
reviewers=None, tbrs=None, add_owners_to=None, reviewers=None, tbrs=None, add_owners_to=None,
expected_description='desc'): expected_description='desc'):
...@@ -1525,10 +1528,10 @@ class TestGitCl(unittest.TestCase): ...@@ -1525,10 +1528,10 @@ class TestGitCl(unittest.TestCase):
'b': ['b@example.com'], 'b': ['b@example.com'],
'c': ['c@example.com'], 'c': ['c@example.com'],
} }
mockGetRoot.return_value = 'root' mockIsCodeOwnersEnabled.return_value = True
mockGetBranch.return_value = branch mockGetBranch.return_value = branch
mockGetBugPrefix.return_value = 'prefix' mockGetBugPrefix.return_value = 'prefix'
mockGetCommonAncestorWithUpstream.return_value = 'upstream' mockGetRemoteBranch.return_value = ('origin', 'refs/remotes/origin/main')
mockFetchDescription.return_value = 'desc' mockFetchDescription.return_value = 'desc'
mockBatchListOwners.side_effect = lambda ps: { mockBatchListOwners.side_effect = lambda ps: {
p: owners_by_path.get(p) p: owners_by_path.get(p)
...@@ -4104,8 +4107,18 @@ class CMDOwnersTestCase(CMDTestCaseBase): ...@@ -4104,8 +4107,18 @@ class CMDOwnersTestCase(CMDTestCaseBase):
'git_cl.Changelist.GetCommonAncestorWithUpstream', 'git_cl.Changelist.GetCommonAncestorWithUpstream',
return_value='upstream').start() return_value='upstream').start()
mock.patch( mock.patch(
'owners_client.DepotToolsClient.BatchListOwners', 'git_cl.Changelist.GetGerritHost',
return_value='host').start()
mock.patch(
'git_cl.Changelist.GetGerritProject',
return_value='project').start()
mock.patch(
'git_cl.Changelist.GetRemoteBranch',
return_value=('origin', 'refs/remotes/origin/main')).start()
mock.patch(
'owners_client.OwnersClient.BatchListOwners',
return_value=self.owners_by_path).start() return_value=self.owners_by_path).start()
mock.patch('gerrit_util.IsCodeOwnersEnabled', return_value=True).start()
self.addCleanup(mock.patch.stopall) self.addCleanup(mock.patch.stopall)
def testShowAllNoArgs(self): def testShowAllNoArgs(self):
...@@ -4118,7 +4131,7 @@ class CMDOwnersTestCase(CMDTestCaseBase): ...@@ -4118,7 +4131,7 @@ class CMDOwnersTestCase(CMDTestCaseBase):
self.assertEqual( self.assertEqual(
0, 0,
git_cl.main(['owners', '--show-all', 'foo', 'bar', 'baz'])) git_cl.main(['owners', '--show-all', 'foo', 'bar', 'baz']))
owners_client.DepotToolsClient.BatchListOwners.assert_called_once_with( owners_client.OwnersClient.BatchListOwners.assert_called_once_with(
['foo', 'bar', 'baz']) ['foo', 'bar', 'baz'])
self.assertEqual( self.assertEqual(
'\n'.join([ '\n'.join([
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment