From 295dd1879f8ed3ac984347a4561f2b34f66dac83 Mon Sep 17 00:00:00 2001 From: Edward Lesmes Date: Tue, 24 Nov 2020 23:07:26 +0000 Subject: [PATCH] [owners] Add SuggestOwners to OwnersClient. Gerrit API doesn't provide the score for an owner of a path, so we can't use the same algorithm when suggesting owners. This change introduces a new algorithm to select the smallest set of at least 2 owners that can approve the change. Change-Id: If620073bdf63633f171c1480e345dbaf75e9f575 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2556479 Reviewed-by: Josip Sokcevic Commit-Queue: Edward Lesmes --- owners_client.py | 54 ++++++++++++++++++++++++++ tests/owners_client_test.py | 76 ++++++++++++++++++++++++++++++++++++- 2 files changed, 129 insertions(+), 1 deletion(-) diff --git a/owners_client.py b/owners_client.py index 783ae9d9c9..4cc48a9f0a 100644 --- a/owners_client.py +++ b/owners_client.py @@ -2,6 +2,7 @@ # Use of this source code is governed by a BSD-style license that can be # found in the LICENSE file. +import itertools import os import random @@ -15,6 +16,29 @@ PENDING = 'PENDING' INSUFFICIENT_REVIEWERS = 'INSUFFICIENT_REVIEWERS' +def _owner_combinations(owners, num_owners): + """Iterate owners combinations by decrasing score. + + The score of an owner is its position on the owners list. + The score of a set of owners is the maximum score of all owners on the set. + + Returns all combinations of up to `num_owners` sorted by decreasing score: + _owner_combinations(['0', '1', '2', '3'], 2) == [ + # score 1 + ('1', '0'), + # score 2 + ('2', '0'), + ('2', '1'), + # score 3 + ('3', '0'), + ('3', '1'), + ('3', '2'), + ] + """ + return reversed(list(itertools.combinations(reversed(owners), num_owners))) + + + class InvalidOwnersConfig(Exception): pass @@ -80,6 +104,36 @@ class OwnersClient(object): status[path] = INSUFFICIENT_REVIEWERS return status + def SuggestOwners(self, project, branch, 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) + 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 + # owners that appear first should be prefered. + # We define the score of an owner to be their minimum position in all + # 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]) + + # Select the minimum number of owners that can approve all paths. + # We start at 2 to avoid sending all changes that require multiple reviewers + # to top-level owners. + num_owners = 2 + while True: + # Iterate all combinations of `num_owners` by decreasing score, and select + # the first one that covers all paths. + 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 + num_owners += 1 + class DepotToolsClient(OwnersClient): """Implement OwnersClient using owners.py Database.""" diff --git a/tests/owners_client_test.py b/tests/owners_client_test.py index 03f127d930..fc58c90c5f 100644 --- a/tests/owners_client_test.py +++ b/tests/owners_client_test.py @@ -14,12 +14,18 @@ else: sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) import gerrit_util -import owners import owners_client from testing_support import filesystem_mock +alice = 'alice@example.com' +bob = 'bob@example.com' +chris = 'chris@example.com' +dave = 'dave@example.com' +emily = 'emily@example.com' + + def _get_change(): return { "labels": { @@ -144,6 +150,74 @@ class OwnersClientTest(unittest.TestCase): 'insufficient': owners_client.INSUFFICIENT_REVIEWERS, }) + def test_owner_combinations(self): + owners = [alice, bob, chris, dave, emily] + self.assertEqual( + list(owners_client._owner_combinations(owners, 2)), + [(bob, alice), + (chris, alice), + (chris, bob), + (dave, alice), + (dave, bob), + (dave, chris), + (emily, alice), + (emily, bob), + (emily, chris), + (emily, dave)]) + + def testSuggestOwners(self): + self.client.owners_by_path = {'abcd': [alice, bob, chris, dave]} + self.assertEqual( + self.client.SuggestOwners('project', 'branch', ['abcd']), + (bob, alice)) + + self.client.owners_by_path = { + 'ae': [alice, emily], + 'be': [bob, emily], + 'ce': [chris, emily], + 'de': [dave, emily], + } + self.assertEqual( + self.client.SuggestOwners( + 'project', 'branch', ['ae', 'be', 'ce', 'de']), + (emily, bob)) + + self.client.owners_by_path = { + 'ad': [alice, dave], + 'cad': [chris, alice, dave], + 'ead': [emily, alice, dave], + 'bd': [bob, dave], + } + self.assertEqual( + self.client.SuggestOwners( + 'project', 'branch', ['ad', 'cad', 'ead', 'bd']), + (bob, alice)) + + self.client.owners_by_path = { + 'a': [alice], + 'b': [bob], + 'c': [chris], + 'ad': [alice, dave], + } + self.assertEqual( + self.client.SuggestOwners( + 'project', 'branch', ['a', 'b', 'c', 'ad']), + (alice, chris, bob)) + + self.client.owners_by_path = { + 'abc': [alice, bob, chris], + 'acb': [alice, chris, bob], + 'bac': [bob, alice, chris], + 'bca': [bob, chris, alice], + 'cab': [chris, alice, bob], + 'cba': [chris, bob, alice] + } + self.assertEqual( + self.client.SuggestOwners( + 'project', 'branch', + ['abc', 'acb', 'bac', 'bca', 'cab', 'cba']), + (chris, bob)) + if __name__ == '__main__': unittest.main()