From b472168267a0eecf36cded969fb681b651b9cbbc Mon Sep 17 00:00:00 2001 From: Edward Lesmes Date: Thu, 19 Nov 2020 22:59:57 +0000 Subject: [PATCH] [owners] Validate owners config in Depot Tools client. Change-Id: Ia9cb19d2b1f776c41ef7e96e6a0ebfb0fcd41344 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2548029 Auto-Submit: Edward Lesmes Commit-Queue: Anthony Polito Reviewed-by: Anthony Polito --- owners_client.py | 25 ++++++++++- tests/owners_client_test.py | 88 +++++++++++++++++++++---------------- 2 files changed, 74 insertions(+), 39 deletions(-) diff --git a/owners_client.py b/owners_client.py index da259a3c0..3d15020fc 100644 --- a/owners_client.py +++ b/owners_client.py @@ -14,6 +14,10 @@ PENDING = 'PENDING' INSUFFICIENT_REVIEWERS = 'INSUFFICIENT_REVIEWERS' +class InvalidOwnersConfig(Exception): + pass + + class OwnersClient(object): """Interact with OWNERS files in a repository. @@ -36,7 +40,7 @@ class OwnersClient(object): raise Exception('Not implemented') def GetChangeApprovalStatus(self, change_id): - """Check the approval status for the latest patch in a change. + """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. @@ -46,7 +50,7 @@ class OwnersClient(object): """ raise Exception('Not implemented') - def IsOwnerConfigurationValid(self, change_id, patch): + def ValidateOwnersConfig(self, change_id): """Check if the owners configuration in a change is valid.""" raise Exception('Not implemented') @@ -56,6 +60,8 @@ class DepotToolsClient(OwnersClient): def __init__(self, host, root, fopen, os_path, branch): super(DepotToolsClient, self).__init__(host) self._root = root + self._fopen = fopen + self._os_path = os_path self._branch = branch self._db = owners.Database(root, fopen, os_path) self._db.override_files = self._GetOriginalOwnersFiles() @@ -96,3 +102,18 @@ class DepotToolsClient(OwnersClient): else: status[f] = INSUFFICIENT_REVIEWERS return status + + 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.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) diff --git a/tests/owners_client_test.py b/tests/owners_client_test.py index 74cccc5b0..691c24ed9 100644 --- a/tests/owners_client_test.py +++ b/tests/owners_client_test.py @@ -20,39 +20,40 @@ import owners_client from testing_support import filesystem_mock -_TEST_CHANGE = { - "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" +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": {}, + "reviewers": { + "REVIEWER": [ + {"email": "approver@example.com"}, + {"email": "reviewer@example.com"}, + ], + }, + "current_revision": "cb90826d03533d6c4e1f0e974ebcbfd7a6f42406", + "revisions": { + "cb90826d03533d6c4e1f0e974ebcbfd7a6f42406": { + "files": { + "approved.cc": {}, + "reviewed.h": {}, + "bar/insufficient.py": {}, + }, }, }, - }, -} + } @@ -70,32 +71,45 @@ class DepotToolsClientTest(unittest.TestCase): '/bar/everyone/OWNERS': '*', '/bar/everyone/foo.txt': '', }) - self.files = self.repo.files self.root = '/' self.fopen = self.repo.open_for_reading mock.patch( 'owners_client.DepotToolsClient._GetOriginalOwnersFiles', return_value={}).start() self.addCleanup(mock.patch.stopall) + self.client = owners_client.DepotToolsClient( + 'host', '/', self.fopen, self.repo, 'branch') def testListOwners(self): - client = owners_client.DepotToolsClient( - 'host', '/', self.fopen, self.repo, 'branch') self.assertEquals( ['*', 'missing@example.com'], - client.ListOwnersForFile('project', 'branch', 'bar/everyone/foo.txt')) + self.client.ListOwnersForFile( + 'project', 'branch', 'bar/everyone/foo.txt')) - @mock.patch('gerrit_util.GetChange', return_value=_TEST_CHANGE) + @mock.patch('gerrit_util.GetChange', return_value=_get_change()) def testGetChangeApprovalStatus(self, _mock): - client = owners_client.DepotToolsClient( - 'host', '/', self.fopen, self.repo, 'branch') self.assertEquals( { 'approved.cc': owners_client.APPROVED, 'reviewed.h': owners_client.PENDING, 'bar/insufficient.py': owners_client.INSUFFICIENT_REVIEWERS, }, - client.GetChangeApprovalStatus('changeid')) + 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') if __name__ == '__main__':