[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 <ehmaldonado@chromium.org>
Reviewed-by: Gavin Mak <gavinmak@google.com>
diff --git a/owners_client.py b/owners_client.py
index 5dabe5d..6f6ecf2 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 @@
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 @@
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]}."""
+ def BatchListOwners(self, paths):
+ """List all owners for a group of files.
+
+ 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))
+ lambda p: (p, self.ListOwners(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 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):
+ 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 @@
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 @@
# 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 @@
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 @@
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__()
- def ListOwnersForFile(self, project, branch, path):
+ self._host = host
+ self._project = project
+ self._branch = branch
+
+ 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]