diff --git a/doc/source/actions.rst b/doc/source/actions.rst index 16cc34674d..f9d9d8d7ea 100644 --- a/doc/source/actions.rst +++ b/doc/source/actions.rst @@ -494,13 +494,21 @@ request. - Default - Value Description * - ``users`` - - list of string + - list of string or dictionary of login and weight - - The username to request reviews from. * - ``teams`` - - list of string + - list of string or dictionary of login and weight - - The team name to request reviews from. + * - ``random_count`` + - integer between 1 and 15 + - + - Pick random users and teams from the provided lists. When + ``random_count`` is specified, ``users`` and ``teams`` can be a + dictionary where the key is the login and the value is the weight to use. + + |essential plan tag| .. note:: diff --git a/mergify_engine/actions/request_reviews.py b/mergify_engine/actions/request_reviews.py index 1ebaf70f7c..4efc3d4b70 100644 --- a/mergify_engine/actions/request_reviews.py +++ b/mergify_engine/actions/request_reviews.py @@ -1,5 +1,3 @@ -# -*- encoding: utf-8 -*- -# # Copyright © 2019–2020 Mergify SAS # # Licensed under the Apache License, Version 2.0 (the "License"); you may @@ -18,6 +16,8 @@ import voluptuous from mergify_engine import actions +from mergify_engine import subscription +from mergify_engine import utils from mergify_engine.clients import http from mergify_engine.rules import types @@ -29,14 +29,32 @@ class RequestReviewsAction(actions.Action): # Any review passed that number is ignored by GitHub API. GITHUB_MAXIMUM_REVIEW_REQUEST = 15 + _random_weight = voluptuous.Required( + voluptuous.All(int, voluptuous.Range(min=1, max=65535)), default=1 + ) + validator = { - voluptuous.Required("users", default=[]): voluptuous.All( - [types.GitHubLogin], - voluptuous.Length(max=GITHUB_MAXIMUM_REVIEW_REQUEST), + voluptuous.Required("users", default=[]): voluptuous.Any( + voluptuous.All( + [types.GitHubLogin], + voluptuous.Length(max=GITHUB_MAXIMUM_REVIEW_REQUEST), + ), + { + types.GitHubLogin: _random_weight, + }, + ), + voluptuous.Required("teams", default=[]): voluptuous.Any( + voluptuous.All( + [types.GitHubTeam], + voluptuous.Length(max=GITHUB_MAXIMUM_REVIEW_REQUEST), + ), + { + types.GitHubTeam: _random_weight, + }, ), - voluptuous.Required("teams", default=[]): voluptuous.All( - [types.GitHubTeam], - voluptuous.Length(max=GITHUB_MAXIMUM_REVIEW_REQUEST), + "random_count": voluptuous.All( + int, + voluptuous.Range(1, GITHUB_MAXIMUM_REVIEW_REQUEST), ), } @@ -44,7 +62,71 @@ class RequestReviewsAction(actions.Action): always_run = True + def _get_random_reviewers(self, random_number: int, pr_author: str) -> set: + if isinstance(self.config["users"], dict): + user_weights = self.config["users"] + else: + user_weights = {user: 1 for user in self.config["users"]} + + if isinstance(self.config["teams"], dict): + team_weights = self.config["teams"] + else: + team_weights = {team: 1 for team in self.config["teams"]} + + choices = { + **{user.lower(): weight for user, weight in user_weights.items()}, + **{f"@{team}": weight for team, weight in team_weights.items()}, + } + + try: + del choices[pr_author.lower()] + except KeyError: + pass + + count = min(self.config["random_count"], len(choices)) + + return utils.get_random_choices( + random_number, + choices, + count, + ) + + def _get_reviewers( + self, pr_id: int, existing_reviews: set, pr_author: str + ) -> (set, set): + if "random_count" in self.config: + team_reviews_to_request = set() + user_reviews_to_request = set() + + for reviewer in self._get_random_reviewers(pr_id, pr_author): + if reviewer.startswith("@"): + team_reviews_to_request.add(reviewer[1:]) + else: + user_reviews_to_request.add(reviewer) + else: + user_reviews_to_request = set(self.config["users"]) + team_reviews_to_request = set(self.config["teams"]) + + user_reviews_to_request -= existing_reviews + user_reviews_to_request -= {pr_author} + + # Team starts with @ + team_reviews_to_request -= { + e[1:] for e in existing_reviews if e.startswith("@") + } + + return user_reviews_to_request, team_reviews_to_request + def run(self, ctxt, rule, missing_conditions): + if "random_count" in self.config and not ctxt.subscription.has_feature( + subscription.Features.RANDOM_REQUEST_REVIEWS + ): + return ( + "action_required", + "Random request reviews are disabled", + f"⚠ The [subscription](https://dashboard.mergify.io/installation/{ctxt.client.auth.installation['id']}/subscription) needed to be updated to enable them.", + ) + # Using consolidated data to avoid already done API lookup reviews_keys = ( "approved-reviews-by", @@ -56,15 +138,13 @@ def run(self, ctxt, rule, missing_conditions): existing_reviews = set( itertools.chain(*[getattr(ctxt.pull_request, key) for key in reviews_keys]) ) - user_reviews_to_request = ( - set(self.config["users"]) - - existing_reviews - - set((ctxt.pull["user"]["login"],)) - ) - team_reviews_to_request = set(self.config["teams"]).difference( - # Team starts with @ - {e[1:] for e in existing_reviews if e.startswith("@")} + + user_reviews_to_request, team_reviews_to_request = self._get_reviewers( + ctxt.pull["id"], + existing_reviews, + ctxt.pull["user"]["login"], ) + if user_reviews_to_request or team_reviews_to_request: requested_reviews_nb = len(ctxt.pull_request.review_requested) diff --git a/mergify_engine/subscription.py b/mergify_engine/subscription.py index 487833f06a..13138ec09d 100644 --- a/mergify_engine/subscription.py +++ b/mergify_engine/subscription.py @@ -35,6 +35,7 @@ class Features(enum.Enum): LARGE_REPOSITORY = "large_repository" PRIORITY_QUEUES = "priority_queues" CUSTOM_CHECKS = "custom_checks" + RANDOM_REQUEST_REVIEWS = "random_request_reviews" @dataclasses.dataclass diff --git a/mergify_engine/tests/unit/actions/test_request_reviews.py b/mergify_engine/tests/unit/actions/test_request_reviews.py index bfdfc608f0..65c5772bd8 100644 --- a/mergify_engine/tests/unit/actions/test_request_reviews.py +++ b/mergify_engine/tests/unit/actions/test_request_reviews.py @@ -11,9 +11,13 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. +from unittest import mock + import pytest import voluptuous +from mergify_engine import context +from mergify_engine import subscription from mergify_engine.actions import request_reviews @@ -62,3 +66,157 @@ def test_config_not_ok(config, error): with pytest.raises(voluptuous.MultipleInvalid) as p: request_reviews.RequestReviewsAction.get_schema()(config) assert str(p.value) == error + + +def test_random_reviewers(): + action = request_reviews.RequestReviewsAction.get_schema()( + { + "random_count": 2, + "teams": { + "foobar": 2, + "foobaz": 1, + }, + "users": { + "jd": 2, + "sileht": 1, + }, + }, + ) + reviewers = action._get_random_reviewers(123, "jd") + assert reviewers == {"@foobar", "sileht"} + reviewers = action._get_random_reviewers(124, "sileht") + assert reviewers == {"jd", "@foobar"} + reviewers = action._get_random_reviewers(124, "jd") + assert reviewers == {"@foobaz", "@foobar"} + + +def test_random_reviewers_no_weight(): + action = request_reviews.RequestReviewsAction.get_schema()( + { + "random_count": 2, + "teams": { + "foobar": 2, + "foobaz": 1, + }, + "users": ["jd", "sileht"], + }, + ) + reviewers = action._get_random_reviewers(123, "another-jd") + assert reviewers == {"sileht", "jd"} + reviewers = action._get_random_reviewers(124, "another-jd") + assert reviewers == {"sileht", "@foobar"} + reviewers = action._get_random_reviewers(124, "sileht") + assert reviewers == {"@foobaz", "@foobar"} + + +def test_random_reviewers_count_bigger(): + action = request_reviews.RequestReviewsAction.get_schema()( + { + "random_count": 15, + "teams": { + "foobar": 2, + "foobaz": 1, + }, + "users": { + "jd": 2, + "sileht": 45, + }, + } + ) + reviewers = action._get_random_reviewers(123, "foobar") + assert reviewers == {"@foobar", "@foobaz", "jd", "sileht"} + reviewers = action._get_random_reviewers(124, "another-jd") + assert reviewers == {"@foobar", "@foobaz", "jd", "sileht"} + reviewers = action._get_random_reviewers(124, "jd") + assert reviewers == {"@foobar", "@foobaz", "sileht"} + + +def test_random_config_too_much_count(): + with pytest.raises(voluptuous.MultipleInvalid) as p: + request_reviews.RequestReviewsAction.get_schema()( + { + "random_count": 20, + "teams": { + "foobar": 2, + "foobaz": 1, + }, + "users": { + "foobar": 2, + "foobaz": 1, + }, + }, + ) + assert ( + str(p.value) + == "value must be at most 15 for dictionary value @ data['random_count']" + ) + + +def test_get_reviewers(): + action = request_reviews.RequestReviewsAction.get_schema()( + { + "random_count": 2, + "teams": { + "foobar": 2, + "foobaz": 1, + }, + "users": { + "jd": 2, + "sileht": 1, + }, + }, + ) + reviewers = action._get_reviewers(843, set(), "another-jd") + assert reviewers == ({"jd", "sileht"}, set()) + reviewers = action._get_reviewers(844, set(), "another-jd") + assert reviewers == ({"jd"}, {"foobar"}) + reviewers = action._get_reviewers(845, set(), "another-jd") + assert reviewers == ({"sileht"}, {"foobar"}) + reviewers = action._get_reviewers(845, {"sileht"}, "another-jd") + assert reviewers == (set(), {"foobar"}) + reviewers = action._get_reviewers(845, {"jd"}, "another-jd") + assert reviewers == ({"sileht"}, {"foobar"}) + reviewers = action._get_reviewers(845, set(), "SILEHT") + assert reviewers == ({"jd"}, {"foobar"}) + + +def test_disabled(): + action = request_reviews.RequestReviewsAction.get_schema()( + { + "random_count": 2, + "teams": { + "foobar": 2, + "foobaz": 1, + }, + "users": { + "jd": 2, + "sileht": 1, + }, + }, + ) + client = mock.MagicMock() + client.auth.installation.__getitem__.return_value = 123 + sub = subscription.Subscription( + 123, + False, + "No sub", + {}, + frozenset({}), + ) + ctxt = context.Context( + client, + { + "number": 123, + "state": None, + "mergeable_state": "ok", + "merged_by": None, + "merged": None, + "merged_at": None, + }, + sub, + ) + assert action.run(ctxt, None, None) == ( + "action_required", + "Random request reviews are disabled", + "⚠ The [subscription](https://dashboard.mergify.io/installation/123/subscription) needed to be updated to enable them.", + ) diff --git a/mergify_engine/tests/unit/flycheck_test_utils.py b/mergify_engine/tests/unit/flycheck_test_utils.py new file mode 100644 index 0000000000..b307fcada1 --- /dev/null +++ b/mergify_engine/tests/unit/flycheck_test_utils.py @@ -0,0 +1,79 @@ +# +# Copyright © 2019–2020 Mergify SAS +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +import pytest + +from mergify_engine import utils + + +def test_unicode_truncate(): + s = "hé ho! how are you√2?" + assert utils.unicode_truncate(s, 0) == "" + assert utils.unicode_truncate(s, 1) == "h" + assert utils.unicode_truncate(s, 2) == "h" + assert utils.unicode_truncate(s, 3) == "hé" + assert utils.unicode_truncate(s, 4) == "hé " + assert utils.unicode_truncate(s, 10) == "hé ho! ho" + assert utils.unicode_truncate(s, 18) == "hé ho! how are yo" + assert utils.unicode_truncate(s, 19) == "hé ho! how are you" + assert utils.unicode_truncate(s, 20) == "hé ho! how are you" + assert utils.unicode_truncate(s, 21) == "hé ho! how are you" + assert utils.unicode_truncate(s, 22) == "hé ho! how are you√" + assert utils.unicode_truncate(s, 23) == "hé ho! how are you√2" + assert utils.unicode_truncate(s, 50) == s + + +def test_process_identifier(): + assert isinstance(utils._PROCESS_IDENTIFIER, str) + + +def test_get_random_choices(): + choices = { + "jd": 10, + "sileht": 1, + "foobar": 3, + } + assert utils.get_random_choices(0, choices, 1) == {"foobar"} + assert utils.get_random_choices(1, choices, 1) == {"foobar"} + assert utils.get_random_choices(2, choices, 1) == {"foobar"} + assert utils.get_random_choices(3, choices, 1) == {"jd"} + assert utils.get_random_choices(4, choices, 1) == {"jd"} + assert utils.get_random_choices(11, choices, 1) == {"jd"} + assert utils.get_random_choices(12, choices, 1) == {"jd"} + assert utils.get_random_choices(13, choices, 1) == {"sileht"} + assert utils.get_random_choices(14, choices, 1) == {"foobar"} + assert utils.get_random_choices(15, choices, 1) == {"foobar"} + assert utils.get_random_choices(16, choices, 1) == {"foobar"} + assert utils.get_random_choices(17, choices, 1) == {"jd"} + assert utils.get_random_choices(18, choices, 1) == {"jd"} + assert utils.get_random_choices(19, choices, 1) == {"jd"} + assert utils.get_random_choices(20, choices, 1) == {"jd"} + assert utils.get_random_choices(21, choices, 1) == {"jd"} + assert utils.get_random_choices(22, choices, 1) == {"jd"} + assert utils.get_random_choices(23, choices, 1) == {"jd"} + assert utils.get_random_choices(24, choices, 1) == {"jd"} + assert utils.get_random_choices(25, choices, 1) == {"jd"} + assert utils.get_random_choices(26, choices, 1) == {"jd"} + assert utils.get_random_choices(27, choices, 1) == {"sileht"} + assert utils.get_random_choices(28, choices, 1) == {"foobar"} + assert utils.get_random_choices(29, choices, 1) == {"foobar"} + assert utils.get_random_choices(30, choices, 1) == {"foobar"} + assert utils.get_random_choices(31, choices, 1) == {"jd"} + assert utils.get_random_choices(32, choices, 1) == {"jd"} + assert utils.get_random_choices(23, choices, 2) == {"sileht", "jd"} + assert utils.get_random_choices(2, choices, 2) == {"jd", "foobar"} + assert utils.get_random_choices(4, choices, 2) == {"jd", "foobar"} + assert utils.get_random_choices(0, choices, 3) == {"jd", "sileht", "foobar"} + with pytest.raises(ValueError): + assert utils.get_random_choices(4, choices, 4) == {"jd", "sileht"} diff --git a/mergify_engine/tests/unit/test_utils.py b/mergify_engine/tests/unit/test_utils.py index 11f173769b..b307fcada1 100644 --- a/mergify_engine/tests/unit/test_utils.py +++ b/mergify_engine/tests/unit/test_utils.py @@ -1,3 +1,19 @@ +# +# Copyright © 2019–2020 Mergify SAS +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +import pytest + from mergify_engine import utils @@ -20,3 +36,44 @@ def test_unicode_truncate(): def test_process_identifier(): assert isinstance(utils._PROCESS_IDENTIFIER, str) + + +def test_get_random_choices(): + choices = { + "jd": 10, + "sileht": 1, + "foobar": 3, + } + assert utils.get_random_choices(0, choices, 1) == {"foobar"} + assert utils.get_random_choices(1, choices, 1) == {"foobar"} + assert utils.get_random_choices(2, choices, 1) == {"foobar"} + assert utils.get_random_choices(3, choices, 1) == {"jd"} + assert utils.get_random_choices(4, choices, 1) == {"jd"} + assert utils.get_random_choices(11, choices, 1) == {"jd"} + assert utils.get_random_choices(12, choices, 1) == {"jd"} + assert utils.get_random_choices(13, choices, 1) == {"sileht"} + assert utils.get_random_choices(14, choices, 1) == {"foobar"} + assert utils.get_random_choices(15, choices, 1) == {"foobar"} + assert utils.get_random_choices(16, choices, 1) == {"foobar"} + assert utils.get_random_choices(17, choices, 1) == {"jd"} + assert utils.get_random_choices(18, choices, 1) == {"jd"} + assert utils.get_random_choices(19, choices, 1) == {"jd"} + assert utils.get_random_choices(20, choices, 1) == {"jd"} + assert utils.get_random_choices(21, choices, 1) == {"jd"} + assert utils.get_random_choices(22, choices, 1) == {"jd"} + assert utils.get_random_choices(23, choices, 1) == {"jd"} + assert utils.get_random_choices(24, choices, 1) == {"jd"} + assert utils.get_random_choices(25, choices, 1) == {"jd"} + assert utils.get_random_choices(26, choices, 1) == {"jd"} + assert utils.get_random_choices(27, choices, 1) == {"sileht"} + assert utils.get_random_choices(28, choices, 1) == {"foobar"} + assert utils.get_random_choices(29, choices, 1) == {"foobar"} + assert utils.get_random_choices(30, choices, 1) == {"foobar"} + assert utils.get_random_choices(31, choices, 1) == {"jd"} + assert utils.get_random_choices(32, choices, 1) == {"jd"} + assert utils.get_random_choices(23, choices, 2) == {"sileht", "jd"} + assert utils.get_random_choices(2, choices, 2) == {"jd", "foobar"} + assert utils.get_random_choices(4, choices, 2) == {"jd", "foobar"} + assert utils.get_random_choices(0, choices, 3) == {"jd", "sileht", "foobar"} + with pytest.raises(ValueError): + assert utils.get_random_choices(4, choices, 4) == {"jd", "sileht"} diff --git a/mergify_engine/utils.py b/mergify_engine/utils.py index 449a35a3ee..b0bbacb165 100644 --- a/mergify_engine/utils.py +++ b/mergify_engine/utils.py @@ -1,4 +1,5 @@ -# -*- encoding: utf-8 -*- +# +# Copyright © 2019–2020 Mergify SAS # # Licensed under the Apache License, Version 2.0 (the "License"); you may # not use this file except in compliance with the License. You may obtain @@ -11,7 +12,6 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. - import asyncio import datetime import hashlib @@ -21,6 +21,7 @@ import socket import subprocess import tempfile +import typing import urllib.parse import aredis @@ -143,3 +144,40 @@ def add_cred(self, username, password, path): parsed[2] = path url = urllib.parse.urlunparse(parsed) self("credential", "approve", input=f"url={url}\n\n".encode("utf8")) + + +def get_random_choices( + random_number: int, population: typing.Dict[typing.Any, int], k: int = 1 +) -> set: + """Return a random number of item from a population without replacement. + + You need to provide the random number yourself. + + The output is always the same based on that number. + + The population is a dict where the key is the choice and the value is the weight. + + The argument k is the number of item that should be picked. + + :param random_number: The random_number that should be picked. + :param population: The dict of {item: weight}. + :param k: The number of choices to make. + :return: A set with the choices. + """ + picked = set() + population = population.copy() + + if k > len(population): + raise ValueError("k cannot be greater than the population size") + + while len(picked) < k: + total_weight = sum(population.values()) + choice_index = (random_number % total_weight) + 1 + for item in sorted(population): + choice_index -= population[item] + if choice_index <= 0: + picked.add(item) + del population[item] + break + + return picked