diff --git a/git_cl.py b/git_cl.py index 5f9dba59d..c6fdd9894 100755 --- a/git_cl.py +++ b/git_cl.py @@ -42,7 +42,6 @@ import git_footers import git_new_branch import metrics import metrics_utils -import owners import owners_client import owners_finder import presubmit_canned_checks @@ -1376,14 +1375,11 @@ class Changelist(object): add_owners = [] if options.add_owners_to: # Fill gaps in OWNERS coverage to tbrs/reviewers if requested. - project = self.GetGerritProject() - branch = self.GetCommonAncestorWithUpstream() client = owners_client.DepotToolsClient( - host=self.GetGerritHost(), root=settings.GetRoot(), - branch=branch) + branch=self.GetCommonAncestorWithUpstream()) status = client.GetFilesApprovalStatus( - project, branch, files, [], options.tbrs + options.reviewers) + files, [], options.tbrs + options.reviewers) missing_files = [ f for f in files if status[f] == owners_client.INSUFFICIENT_REVIEWERS @@ -4789,16 +4785,13 @@ def CMDowners(parser, args): if len(args) == 0: print('No files specified for --show-all. Nothing to do.') return 0 - project = cl.GetGerritProject() - branch = cl.GetCommonAncestorWithUpstream() client = owners_client.DepotToolsClient( - host=cl.GetGerritHost(), root=settings.GetRoot(), - branch=branch) - for arg in args: - print('Owners for %s:' % arg) - for owner in client.ListOwnersForFile(project, branch, arg): - print(' - %s' % owner) + branch=cl.GetCommonAncestorWithUpstream()) + owners_by_file = client.BatchListOwners(args) + for f, owners in owners_by_file.items(): + print('Owners for %s:' % f) + print('\n'.join(' - %s' % owner for owner in owners)) return 0 if args: @@ -4813,13 +4806,10 @@ def CMDowners(parser, args): affected_files = cl.GetAffectedFiles(base_branch) if options.batch: - project = cl.GetGerritProject() - branch = cl.GetCommonAncestorWithUpstream() client = owners_client.DepotToolsClient( - host=cl.GetGerritHost(), root=settings.GetRoot(), - branch=branch) - print('\n'.join(client.SuggestOwners(project, branch, affected_files))) + branch=cl.GetCommonAncestorWithUpstream()) + print('\n'.join(client.SuggestOwners(affected_files))) return 0 owner_files = [f for f in affected_files if 'OWNERS' in os.path.basename(f)] diff --git a/owners_client.py b/owners_client.py index 5dabe5dc0..6f6ecf29c 100644 --- a/owners_client.py +++ b/owners_client.py @@ -5,6 +5,7 @@ import itertools import os import random +import threading import gerrit_util import git_common @@ -39,10 +40,6 @@ def _owner_combinations(owners, num_owners): return reversed(list(itertools.combinations(reversed(owners), num_owners))) -class InvalidOwnersConfig(Exception): - pass - - class OwnersClient(object): """Interact with OWNERS files in a repository. @@ -50,46 +47,30 @@ class OwnersClient(object): Gerrit Code-Owners plugin REST API, and the owners database implemented by Depot Tools in owners.py: - - List all the owners for a change. - - Check if a change has been approved. - - Check if the OWNERS configuration in a change is valid. + - List all the owners for a group of files. + - Check if files have been approved. + - Suggest owners for a group of files. All code should use this class to interact with OWNERS files instead of the owners database in owners.py """ - def __init__(self, host): - self._host = host - - def ListOwnersForFile(self, project, branch, path): + def ListOwners(self, path): """List all owners for a file. The returned list is sorted so that better owners appear first. """ raise Exception('Not implemented') - def BatchListOwners(self, project, branch, paths): - """Returns a dictionary {path: [owners]}.""" - with git_common.ScopedPool(kind='threads') as pool: - return dict(pool.imap_unordered( - lambda p: (p, self.ListOwnersForFile(project, branch, p)), paths)) - - def GetChangeApprovalStatus(self, change_id): - """Check the approval status for the latest revision_id in a change. + def BatchListOwners(self, paths): + """List all owners for a group of files. - Returns a map of path to approval status, where the status can be one of: - - APPROVED: An owner of the file has reviewed the change. - - PENDING: An owner of the file has been added as a reviewer, but no owner - has approved. - - INSUFFICIENT_REVIEWERS: No owner of the file has been added as a reviewer. + Returns a dictionary {path: [owners]}. """ - raise Exception('Not implemented') - - def ValidateOwnersConfig(self, change_id): - """Check if the owners configuration in a change is valid.""" - raise Exception('Not implemented') + with git_common.ScopedPool(kind='threads') as pool: + return dict(pool.imap_unordered( + lambda p: (p, self.ListOwners(p)), paths)) - def GetFilesApprovalStatus( - self, project, branch, paths, approvers, reviewers): + def GetFilesApprovalStatus(self, paths, approvers, reviewers): """Check the approval status for the given paths. Utility method to check for approval status when a change has not yet been @@ -100,22 +81,23 @@ class OwnersClient(object): approvers = set(approvers) reviewers = set(reviewers) status = {} - for path in paths: - path_owners = set(self.ListOwnersForFile(project, branch, path)) - if path_owners.intersection(approvers): + owners_by_path = self.BatchListOwners(paths) + for path, owners in owners_by_path.items(): + owners = set(owners) + if owners.intersection(approvers): status[path] = APPROVED - elif path_owners.intersection(reviewers): + elif owners.intersection(reviewers): status[path] = PENDING else: status[path] = INSUFFICIENT_REVIEWERS return status - def SuggestOwners(self, project, branch, paths): + def SuggestOwners(self, paths): """Suggest a set of owners for the given paths.""" paths_by_owner = {} score_by_owner = {} - for path in paths: - owners = self.ListOwnersForFile(project, branch, path) + owners_by_path = self.BatchListOwners(paths) + for path, owners in owners_by_path.items(): for i, owner in enumerate(owners): paths_by_owner.setdefault(owner, set()).add(path) # Gerrit API lists owners of a path sorted by an internal score, so @@ -124,8 +106,10 @@ class OwnersClient(object): # paths. score_by_owner[owner] = min(i, score_by_owner.get(owner, i)) - # Sort owners by their score. - owners = sorted(score_by_owner, key=lambda o: score_by_owner[o]) + # Sort owners by their score. Randomize order of owners with same score. + owners = sorted( + score_by_owner, + key=lambda o: (score_by_owner[o], random.random())) # Select the minimum number of owners that can approve all paths. # We start at 2 to avoid sending all changes that require multiple reviewers @@ -139,19 +123,22 @@ class OwnersClient(object): for selected in _owner_combinations(owners, num_owners): covered = set.union(*(paths_by_owner[o] for o in selected)) if len(covered) == len(paths): - return selected + return list(selected) class DepotToolsClient(OwnersClient): """Implement OwnersClient using owners.py Database.""" - def __init__(self, host, root, branch, fopen=open, os_path=os.path): - super(DepotToolsClient, self).__init__(host) + def __init__(self, root, branch, fopen=open, os_path=os.path): + super(DepotToolsClient, self).__init__() + self._root = root + self._branch = branch self._fopen = fopen self._os_path = os_path - self._branch = branch + self._db = owners_db.Database(root, fopen, os_path) self._db.override_files = self._GetOriginalOwnersFiles() + self._db_lock = threading.Lock() def _GetOriginalOwnersFiles(self): return { @@ -160,57 +147,33 @@ class DepotToolsClient(OwnersClient): if os.path.basename(f) == 'OWNERS' } - def ListOwnersForFile(self, _project, _branch, path): - # all_possible_owners returns a dict {owner: [(path, distance)]}. We want to - # return a list of owners sorted by increasing distance. - distance_by_owner = self._db.all_possible_owners([path], None) - # We add a small random number to the distance, so that owners at the same - # distance are returned in random order to avoid overloading those who would - # appear first. - return sorted( - distance_by_owner, - key=lambda o: distance_by_owner[o][0][1] + random.random()) - - def GetChangeApprovalStatus(self, change_id): - data = gerrit_util.GetChange( - self._host, change_id, - ['DETAILED_ACCOUNTS', 'DETAILED_LABELS', 'CURRENT_FILES', - 'CURRENT_REVISION']) - - reviewers = [r['email'] for r in data['reviewers']['REVIEWER']] - - # Get reviewers that have approved this change - label = data['labels']['Code-Review'] - max_value = max(int(v) for v in label['values']) - approvers = [v['email'] for v in label['all'] if v['value'] == max_value] - - files = data['revisions'][data['current_revision']]['files'] - return self.GetFilesApprovalStatus(None, None, files, approvers, reviewers) - - def ValidateOwnersConfig(self, change_id): - data = gerrit_util.GetChange( - self._host, change_id, - ['DETAILED_ACCOUNTS', 'DETAILED_LABELS', 'CURRENT_FILES', - 'CURRENT_REVISION']) - - files = data['revisions'][data['current_revision']]['files'] - - db = owners_db.Database(self._root, self._fopen, self._os_path) - try: - db.load_data_needed_for( - [f for f in files if os.path.basename(f) == 'OWNERS']) - except Exception as e: - raise InvalidOwnersConfig('Error parsing OWNERS files:\n%s' % e) + def ListOwners(self, path): + # all_possible_owners is not thread safe. + with self._db_lock: + # all_possible_owners returns a dict {owner: [(path, distance)]}. We want + # to return a list of owners sorted by increasing distance. + distance_by_owner = self._db.all_possible_owners([path], None) + # We add a small random number to the distance, so that owners at the same + # distance are returned in random order to avoid overloading those who + # would appear first. + return sorted( + distance_by_owner, + key=lambda o: distance_by_owner[o][0][1] + random.random()) class GerritClient(OwnersClient): """Implement OwnersClient using OWNERS REST API.""" - def __init__(self, host): - super(GerritClient, self).__init__(host) + def __init__(self, host, project, branch): + super(GerritClient, self).__init__() + + self._host = host + self._project = project + self._branch = branch - def ListOwnersForFile(self, project, branch, path): + def ListOwners(self, path): # GetOwnersForFile returns a list of account details sorted by order of # best reviewer for path. If code owners have the same score, the order is # random. - data = gerrit_util.GetOwnersForFile(self._host, project, branch, path) + data = gerrit_util.GetOwnersForFile( + self._host, self._project, self._branch, path) return [d['account']['email'] for d in data] diff --git a/tests/owners_client_test.py b/tests/owners_client_test.py index f80bdcec7..606b615b4 100644 --- a/tests/owners_client_test.py +++ b/tests/owners_client_test.py @@ -26,43 +26,6 @@ dave = 'dave@example.com' emily = 'emily@example.com' -def _get_change(): - return { - "labels": { - "Code-Review": { - "all": [ - { - "value": 1, - "email": "approver@example.com", - } - ], - "values": { - "-1": "Don't submit as-is", - " 0": "No score", - "+1": "Looks good to me" - }, - }, - }, - "reviewers": { - "REVIEWER": [ - {"email": "approver@example.com"}, - {"email": "reviewer@example.com"}, - ], - }, - "current_revision": "cb90826d03533d6c4e1f0e974ebcbfd7a6f42406", - "revisions": { - "cb90826d03533d6c4e1f0e974ebcbfd7a6f42406": { - "files": { - "approved.cc": {}, - "reviewed.h": {}, - "bar/insufficient.py": {}, - }, - }, - }, - } - - - def _get_owners(): return [ { @@ -105,65 +68,38 @@ class DepotToolsClientTest(unittest.TestCase): return_value={}).start() self.addCleanup(mock.patch.stopall) self.client = owners_client.DepotToolsClient( - 'host', '/', 'branch', self.fopen, self.repo) + '/', 'branch', self.fopen, self.repo) def testListOwners(self): - self.assertEquals( + self.assertEqual( ['*', 'missing@example.com'], - self.client.ListOwnersForFile( - 'project', 'branch', 'bar/everyone/foo.txt')) - - @mock.patch('gerrit_util.GetChange', return_value=_get_change()) - def testGetChangeApprovalStatus(self, _mock): - self.assertEquals( - { - 'approved.cc': owners_client.APPROVED, - 'reviewed.h': owners_client.PENDING, - 'bar/insufficient.py': owners_client.INSUFFICIENT_REVIEWERS, - }, - self.client.GetChangeApprovalStatus('changeid')) - - @mock.patch('gerrit_util.GetChange', return_value=_get_change()) - def testValidateOwnersConfig_OK(self, get_change_mock): - self.client.ValidateOwnersConfig('changeid') - - @mock.patch('gerrit_util.GetChange', return_value=_get_change()) - def testValidateOwnersConfig_Invalid(self, get_change_mock): - change = get_change_mock() - change['revisions'][change['current_revision']]['files'] = {'/OWNERS': {}} - self.repo.files['/OWNERS'] = '\n'.join([ - 'foo@example.com', - 'invalid directive', - ]) - with self.assertRaises(owners_client.InvalidOwnersConfig): - self.client.ValidateOwnersConfig('changeid') + self.client.ListOwners('bar/everyone/foo.txt')) class GerritClientTest(unittest.TestCase): def setUp(self): - self.client = owners_client.GerritClient('host') + self.client = owners_client.GerritClient('host', 'project', 'branch') @mock.patch('gerrit_util.GetOwnersForFile', return_value=_get_owners()) - def testListOwners(self, get_owners_mock): + def testListOwners(self, _get_owners_mock): self.assertEquals( ['approver@example.com', 'reviewer@example.com', 'missing@example.com'], - self.client.ListOwnersForFile('project', 'branch', - 'bar/everyone/foo.txt')) + self.client.ListOwners('bar/everyone/foo.txt')) class TestClient(owners_client.OwnersClient): - def __init__(self, host, owners_by_path): - super(TestClient, self).__init__(host) + def __init__(self, owners_by_path): + super(TestClient, self).__init__() self.owners_by_path = owners_by_path - def ListOwnersForFile(self, _project, _branch, path): + def ListOwners(self, path): return self.owners_by_path[path] class OwnersClientTest(unittest.TestCase): def setUp(self): self.owners = {} - self.client = TestClient('host', self.owners) + self.client = TestClient(self.owners) def testGetFilesApprovalStatus(self): self.client.owners_by_path = { @@ -172,7 +108,6 @@ class OwnersClientTest(unittest.TestCase): 'insufficient': ['insufficient@example.com'], } status = self.client.GetFilesApprovalStatus( - 'project', 'branch', ['approved', 'pending', 'insufficient'], ['approver@example.com'], ['reviewer@example.com']) self.assertEqual( @@ -201,13 +136,13 @@ class OwnersClientTest(unittest.TestCase): def testSuggestOwners(self): self.client.owners_by_path = {'a': [alice]} self.assertEqual( - self.client.SuggestOwners('project', 'branch', ['a']), + self.client.SuggestOwners(['a']), [alice]) self.client.owners_by_path = {'abcd': [alice, bob, chris, dave]} self.assertEqual( - self.client.SuggestOwners('project', 'branch', ['abcd']), - (bob, alice)) + sorted(self.client.SuggestOwners(['abcd'])), + [alice, bob]) self.client.owners_by_path = { 'ae': [alice, emily], @@ -215,10 +150,10 @@ class OwnersClientTest(unittest.TestCase): 'ce': [chris, emily], 'de': [dave, emily], } - self.assertEqual( - self.client.SuggestOwners( - 'project', 'branch', ['ae', 'be', 'ce', 'de']), - (emily, bob)) + suggested = self.client.SuggestOwners(['ae', 'be', 'ce', 'de']) + # emily should be selected along with anyone else. + self.assertIn(emily, suggested) + self.assertEqual(2, len(suggested)) self.client.owners_by_path = { 'ad': [alice, dave], @@ -227,9 +162,8 @@ class OwnersClientTest(unittest.TestCase): 'bd': [bob, dave], } self.assertEqual( - self.client.SuggestOwners( - 'project', 'branch', ['ad', 'cad', 'ead', 'bd']), - (bob, alice)) + sorted(self.client.SuggestOwners(['ad', 'cad', 'ead', 'bd'])), + [alice, bob]) self.client.owners_by_path = { 'a': [alice], @@ -238,9 +172,8 @@ class OwnersClientTest(unittest.TestCase): 'ad': [alice, dave], } self.assertEqual( - self.client.SuggestOwners( - 'project', 'branch', ['a', 'b', 'c', 'ad']), - (alice, chris, bob)) + sorted(self.client.SuggestOwners(['a', 'b', 'c', 'ad'])), + [alice, bob, chris]) self.client.owners_by_path = { 'abc': [alice, bob, chris], @@ -250,11 +183,10 @@ class OwnersClientTest(unittest.TestCase): 'cab': [chris, alice, bob], 'cba': [chris, bob, alice] } - self.assertEqual( - self.client.SuggestOwners( - 'project', 'branch', - ['abc', 'acb', 'bac', 'bca', 'cab', 'cba']), - (chris, bob)) + suggested = self.client.SuggestOwners( + ['abc', 'acb', 'bac', 'bca', 'cab', 'cba']) + # Any two owners. + self.assertEqual(2, len(suggested)) def testBatchListOwners(self): self.client.owners_by_path = { @@ -270,7 +202,6 @@ class OwnersClientTest(unittest.TestCase): 'bar/foo/': [bob, chris] }, self.client.BatchListOwners( - 'project', 'branch', ['bar/everyone/foo.txt', 'bar/everyone/bar.txt', 'bar/foo/']))