Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to randomly request a review #1425

Merged
merged 3 commits into from
Sep 30, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions doc/source/actions.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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::

Expand Down
112 changes: 96 additions & 16 deletions mergify_engine/actions/request_reviews.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
# -*- encoding: utf-8 -*-
#
# Copyright © 2019–2020 Mergify SAS
#
# Licensed under the Apache License, Version 2.0 (the "License"); you may
Expand All @@ -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

Expand All @@ -29,22 +29,104 @@ 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),
),
}

silent_report = True

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",
Expand All @@ -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)

Expand Down
1 change: 1 addition & 0 deletions mergify_engine/subscription.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
158 changes: 158 additions & 0 deletions mergify_engine/tests/unit/actions/test_request_reviews.py
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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.",
)
Loading