From b34cd6d8fa4df214f77c8141d2113281e57790cc Mon Sep 17 00:00:00 2001 From: Edward Lesmes Date: Thu, 7 Jan 2021 01:05:41 +0000 Subject: [PATCH] Revert "[owners] Refactor OwnersClient." This reverts commit 9c7f6c25c004fe59187fa0695fbd02cb06f9aa4e. Reason for revert: Not working https://logs.chromium.org/logs/infra-internal/buildbucket/cr-buildbucket.appspot.com/8858807889610091152/+/steps/franky/0/steps/git_cl_upload/0/stdout Original change's description: > [owners] Refactor OwnersClient. > > - Remove GetChangeApprovalStatus and GetFilesApprovalStatus. > - Make host, project and branch part of GerritClient. > - Rename ListOwnersForFile to ListOwners. > > Change-Id: I1f610a64f0217ce54e5ce4a210026535adc1c2e5 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2587268 > Commit-Queue: Edward Lesmes > Reviewed-by: Gavin Mak TBR=ehmaldonado@chromium.org,gavinmak@google.com,infra-scoped@luci-project-accounts.iam.gserviceaccount.com,sokcevic@google.com # Not skipping CQ checks because original CL landed > 1 day ago. Change-Id: I4928df90856526210a4fd4d104e6cc96aa005bc2 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2613925 Commit-Queue: Edward Lesmes Reviewed-by: Stephen Martinis Reviewed-by: Edward Lesmes --- git_cl.py | 30 +++++--- owners_client.py | 141 +++++++++++++++++++++++------------- tests/owners_client_test.py | 119 +++++++++++++++++++++++------- 3 files changed, 203 insertions(+), 87 deletions(-) diff --git a/git_cl.py b/git_cl.py index d3b2ce5c3..9fc56f1db 100755 --- a/git_cl.py +++ b/git_cl.py @@ -42,6 +42,7 @@ import git_footers import git_new_branch import metrics import metrics_utils +import owners import owners_client import owners_finder import presubmit_canned_checks @@ -1375,16 +1376,19 @@ 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=self.GetCommonAncestorWithUpstream()) + branch=branch) status = client.GetFilesApprovalStatus( - files, [], options.tbrs + options.reviewers) + project, branch, files, [], options.tbrs + options.reviewers) missing_files = [ f for f in files if status[f] == owners_client.INSUFFICIENT_REVIEWERS ] - add_owners = client.SuggestOwners(missing_files) + add_owners = client.SuggestOwners(project, branch, missing_files) change_description.update_reviewers( options.reviewers, options.tbrs, options.add_owners_to, add_owners) @@ -4791,13 +4795,16 @@ 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=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)) + branch=branch) + for arg in args: + print('Owners for %s:' % arg) + for owner in client.ListOwnersForFile(project, branch, arg): + print(' - %s' % owner) return 0 if args: @@ -4812,10 +4819,13 @@ 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=cl.GetCommonAncestorWithUpstream()) - print('\n'.join(client.SuggestOwners(affected_files))) + branch=branch) + print('\n'.join(client.SuggestOwners(project, branch, 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 6f6ecf29c..5dabe5dc0 100644 --- a/owners_client.py +++ b/owners_client.py @@ -5,7 +5,6 @@ import itertools import os import random -import threading import gerrit_util import git_common @@ -40,6 +39,10 @@ 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. @@ -47,30 +50,46 @@ 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 group of files. - - Check if files have been approved. - - Suggest owners for a group of files. + - List all the owners for a change. + - Check if a change has been approved. + - Check if the OWNERS configuration in a change is valid. All code should use this class to interact with OWNERS files instead of the owners database in owners.py """ - def ListOwners(self, path): + def __init__(self, host): + self._host = host + + def ListOwnersForFile(self, project, branch, 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, paths): - """List all owners for a group of files. - - Returns a dictionary {path: [owners]}. - """ + 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.ListOwners(p)), paths)) + 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. + + 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. + """ + raise Exception('Not implemented') - def GetFilesApprovalStatus(self, paths, approvers, reviewers): + def ValidateOwnersConfig(self, change_id): + """Check if the owners configuration in a change is valid.""" + raise Exception('Not implemented') + + def GetFilesApprovalStatus( + self, project, branch, 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 @@ -81,23 +100,22 @@ class OwnersClient(object): approvers = set(approvers) reviewers = set(reviewers) status = {} - owners_by_path = self.BatchListOwners(paths) - for path, owners in owners_by_path.items(): - owners = set(owners) - if owners.intersection(approvers): + for path in paths: + path_owners = set(self.ListOwnersForFile(project, branch, path)) + if path_owners.intersection(approvers): status[path] = APPROVED - elif owners.intersection(reviewers): + elif path_owners.intersection(reviewers): status[path] = PENDING else: status[path] = INSUFFICIENT_REVIEWERS return status - def SuggestOwners(self, paths): + def SuggestOwners(self, project, branch, paths): """Suggest a set of owners for the given paths.""" paths_by_owner = {} score_by_owner = {} - owners_by_path = self.BatchListOwners(paths) - for path, owners in owners_by_path.items(): + for path in paths: + owners = self.ListOwnersForFile(project, branch, path) 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 @@ -106,10 +124,8 @@ class OwnersClient(object): # paths. score_by_owner[owner] = min(i, score_by_owner.get(owner, i)) - # 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())) + # Sort owners by their score. + owners = sorted(score_by_owner, key=lambda o: score_by_owner[o]) # Select the minimum number of owners that can approve all paths. # We start at 2 to avoid sending all changes that require multiple reviewers @@ -123,22 +139,19 @@ 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 list(selected) + return selected class DepotToolsClient(OwnersClient): """Implement OwnersClient using owners.py Database.""" - def __init__(self, root, branch, fopen=open, os_path=os.path): - super(DepotToolsClient, self).__init__() - + def __init__(self, host, root, branch, fopen=open, os_path=os.path): + super(DepotToolsClient, self).__init__(host) 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 { @@ -147,33 +160,57 @@ class DepotToolsClient(OwnersClient): if os.path.basename(f) == 'OWNERS' } - 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()) + 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) class GerritClient(OwnersClient): """Implement OwnersClient using OWNERS REST API.""" - def __init__(self, host, project, branch): - super(GerritClient, self).__init__() - - self._host = host - self._project = project - self._branch = branch + def __init__(self, host): + super(GerritClient, self).__init__(host) - def ListOwners(self, path): + def ListOwnersForFile(self, project, branch, 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, self._project, self._branch, path) + data = gerrit_util.GetOwnersForFile(self._host, project, 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 606b615b4..f80bdcec7 100644 --- a/tests/owners_client_test.py +++ b/tests/owners_client_test.py @@ -26,6 +26,43 @@ 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 [ { @@ -68,38 +105,65 @@ class DepotToolsClientTest(unittest.TestCase): return_value={}).start() self.addCleanup(mock.patch.stopall) self.client = owners_client.DepotToolsClient( - '/', 'branch', self.fopen, self.repo) + 'host', '/', 'branch', self.fopen, self.repo) def testListOwners(self): - self.assertEqual( + self.assertEquals( ['*', 'missing@example.com'], - self.client.ListOwners('bar/everyone/foo.txt')) + 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') class GerritClientTest(unittest.TestCase): def setUp(self): - self.client = owners_client.GerritClient('host', 'project', 'branch') + self.client = owners_client.GerritClient('host') @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.ListOwners('bar/everyone/foo.txt')) + self.client.ListOwnersForFile('project', 'branch', + 'bar/everyone/foo.txt')) class TestClient(owners_client.OwnersClient): - def __init__(self, owners_by_path): - super(TestClient, self).__init__() + def __init__(self, host, owners_by_path): + super(TestClient, self).__init__(host) self.owners_by_path = owners_by_path - def ListOwners(self, path): + def ListOwnersForFile(self, _project, _branch, path): return self.owners_by_path[path] class OwnersClientTest(unittest.TestCase): def setUp(self): self.owners = {} - self.client = TestClient(self.owners) + self.client = TestClient('host', self.owners) def testGetFilesApprovalStatus(self): self.client.owners_by_path = { @@ -108,6 +172,7 @@ 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( @@ -136,13 +201,13 @@ class OwnersClientTest(unittest.TestCase): def testSuggestOwners(self): self.client.owners_by_path = {'a': [alice]} self.assertEqual( - self.client.SuggestOwners(['a']), + self.client.SuggestOwners('project', 'branch', ['a']), [alice]) self.client.owners_by_path = {'abcd': [alice, bob, chris, dave]} self.assertEqual( - sorted(self.client.SuggestOwners(['abcd'])), - [alice, bob]) + self.client.SuggestOwners('project', 'branch', ['abcd']), + (bob, alice)) self.client.owners_by_path = { 'ae': [alice, emily], @@ -150,10 +215,10 @@ class OwnersClientTest(unittest.TestCase): 'ce': [chris, emily], 'de': [dave, emily], } - 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.assertEqual( + self.client.SuggestOwners( + 'project', 'branch', ['ae', 'be', 'ce', 'de']), + (emily, bob)) self.client.owners_by_path = { 'ad': [alice, dave], @@ -162,8 +227,9 @@ class OwnersClientTest(unittest.TestCase): 'bd': [bob, dave], } self.assertEqual( - sorted(self.client.SuggestOwners(['ad', 'cad', 'ead', 'bd'])), - [alice, bob]) + self.client.SuggestOwners( + 'project', 'branch', ['ad', 'cad', 'ead', 'bd']), + (bob, alice)) self.client.owners_by_path = { 'a': [alice], @@ -172,8 +238,9 @@ class OwnersClientTest(unittest.TestCase): 'ad': [alice, dave], } self.assertEqual( - sorted(self.client.SuggestOwners(['a', 'b', 'c', 'ad'])), - [alice, bob, chris]) + self.client.SuggestOwners( + 'project', 'branch', ['a', 'b', 'c', 'ad']), + (alice, chris, bob)) self.client.owners_by_path = { 'abc': [alice, bob, chris], @@ -183,10 +250,11 @@ class OwnersClientTest(unittest.TestCase): 'cab': [chris, alice, bob], 'cba': [chris, bob, alice] } - suggested = self.client.SuggestOwners( - ['abc', 'acb', 'bac', 'bca', 'cab', 'cba']) - # Any two owners. - self.assertEqual(2, len(suggested)) + self.assertEqual( + self.client.SuggestOwners( + 'project', 'branch', + ['abc', 'acb', 'bac', 'bca', 'cab', 'cba']), + (chris, bob)) def testBatchListOwners(self): self.client.owners_by_path = { @@ -202,6 +270,7 @@ class OwnersClientTest(unittest.TestCase): 'bar/foo/': [bob, chris] }, self.client.BatchListOwners( + 'project', 'branch', ['bar/everyone/foo.txt', 'bar/everyone/bar.txt', 'bar/foo/']))