[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 <ehmaldonado@chromium.org>
Commit-Queue: Anthony Polito <apolito@google.com>
Reviewed-by: Anthony Polito <apolito@google.com>
diff --git a/owners_client.py b/owners_client.py
index da259a3..3d15020 100644
--- a/owners_client.py
+++ b/owners_client.py
@@ -14,6 +14,10 @@
INSUFFICIENT_REVIEWERS = 'INSUFFICIENT_REVIEWERS'
+class InvalidOwnersConfig(Exception):
+ pass
+
+
class OwnersClient(object):
"""Interact with OWNERS files in a repository.
@@ -36,7 +40,7 @@
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 @@
"""
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 @@
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 @@
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 74cccc5..691c24e 100644
--- a/tests/owners_client_test.py
+++ b/tests/owners_client_test.py
@@ -20,39 +20,40 @@
from testing_support import filesystem_mock
-_TEST_CHANGE = {
- "labels": {
- "Code-Review": {
- "all": [
- {
- "value": 1,
- "email": "approver@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"},
],
- "values": {
- "-1": "Don't submit as-is",
- " 0": "No score",
- "+1": "Looks good to me"
+ },
+ "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 @@
'/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__':
