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.", + )