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 <ehmaldonado@chromium.org>
> Reviewed-by: Gavin Mak <gavinmak@google.com>
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 <ehmaldonado@chromium.org>
Reviewed-by: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
diff --git a/owners_client.py b/owners_client.py
index 6f6ecf2..5dabe5d 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 @@
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 @@
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 GetFilesApprovalStatus(self, paths, approvers, reviewers):
+ 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):
"""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 @@
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 @@
# 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 @@
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 @@
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__()
+ def __init__(self, host):
+ super(GerritClient, self).__init__(host)
- self._host = host
- self._project = project
- self._branch = branch
-
- 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]