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

ref(bot): use reviewDecision instead of custom calculation #762

Merged
merged 3 commits into from
Nov 25, 2021
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
34 changes: 1 addition & 33 deletions bot/kodiak/evaluation.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@

import asyncio
import textwrap
from collections import defaultdict
from collections.abc import Iterable
from dataclasses import dataclass
from typing import Dict, List, MutableMapping, Optional, Sequence, Set, Union
from typing import Dict, List, Optional, Sequence, Set, Union

import inflection
import pydantic
Expand Down Expand Up @@ -45,7 +44,6 @@
Commit,
MergeableState,
MergeStateStatus,
Permission,
PRReview,
PRReviewRequest,
PRReviewState,
Expand Down Expand Up @@ -836,36 +834,6 @@ async def set_status(msg: str, markdown_content: Optional[str] = None) -> None:
# - failing required status checks
# - branch not up to date (should be handled before this)
# - missing required signature
if (
branch_protection.requiresApprovingReviews
and branch_protection.requiredApprovingReviewCount
):
reviews_by_author: MutableMapping[str, List[PRReview]] = defaultdict(list)
for review in sorted(reviews, key=lambda x: x.createdAt):
if review.author.permission not in {Permission.ADMIN, Permission.WRITE}:
continue
reviews_by_author[review.author.login].append(review)

successful_reviews = 0
for author_name, review_list in reviews_by_author.items():
review_state = review_status(review_list)
# blocking review
if review_state == PRReviewState.CHANGES_REQUESTED:
await block_merge(
api, pull_request, f"changes requested by {author_name!r}"
)
return
# successful review
if review_state == PRReviewState.APPROVED:
successful_reviews += 1
# missing required review count
if successful_reviews < branch_protection.requiredApprovingReviewCount:
await block_merge(
api,
pull_request,
f"missing required reviews, have {successful_reviews!r}/{branch_protection.requiredApprovingReviewCount!r}",
)
return

if pull_request.reviewDecision == PullRequestReviewDecision.REVIEW_REQUIRED:
await block_merge(api, pull_request, "missing required reviews")
Expand Down
182 changes: 0 additions & 182 deletions bot/kodiak/tests/evaluation/test_branch_protection.py
Original file line number Diff line number Diff line change
Expand Up @@ -188,136 +188,6 @@ async def test_mergeable_requires_commit_signatures_squash_and_merge() -> None:
assert api.merge.called is False


@pytest.mark.asyncio
async def test_mergeable_missing_required_approving_reviews() -> None:
"""
Don't merge when branch protection requires approving reviews and we don't
have enought approving reviews.
"""
api = create_api()
mergeable = create_mergeable()
pull_request = create_pull_request()
branch_protection = create_branch_protection()

pull_request.mergeStateStatus = MergeStateStatus.BLOCKED
branch_protection.requiresApprovingReviews = True
branch_protection.requiredApprovingReviewCount = 1

await mergeable(
api=api,
pull_request=pull_request,
branch_protection=branch_protection,
reviews=[],
)
assert api.set_status.call_count == 1
assert api.dequeue.call_count == 1
assert "missing required reviews" in api.set_status.calls[0]["msg"]

# verify we haven't tried to update/merge the PR
assert api.update_branch.called is False
assert api.merge.called is False
assert api.queue_for_merge.called is False


@pytest.mark.asyncio
async def test_mergeable_missing_required_approving_reviews_has_review_with_missing_perms() -> None:
"""
Don't merge when branch protection requires approving reviews and we don't have enough reviews. If a reviewer doesn't have permissions we should ignore their review.
"""
api = create_api()
mergeable = create_mergeable()
pull_request = create_pull_request()
branch_protection = create_branch_protection()
review = create_review()

pull_request.mergeStateStatus = MergeStateStatus.BLOCKED
branch_protection.requiresApprovingReviews = True
branch_protection.requiredApprovingReviewCount = 1
review.state = PRReviewState.APPROVED
review.author.permission = Permission.READ

await mergeable(
api=api,
pull_request=pull_request,
branch_protection=branch_protection,
reviews=[review],
)
assert api.set_status.call_count == 1
assert api.dequeue.call_count == 1
assert "missing required reviews" in api.set_status.calls[0]["msg"]

# verify we haven't tried to update/merge the PR
assert api.update_branch.called is False
assert api.merge.called is False
assert api.queue_for_merge.called is False


@pytest.mark.asyncio
async def test_mergeable_missing_required_approving_reviews_changes_requested() -> None:
"""
Don't merge when branch protection requires approving reviews and a user requested changes.
"""
api = create_api()
mergeable = create_mergeable()
pull_request = create_pull_request()
branch_protection = create_branch_protection()
review = create_review()

pull_request.mergeStateStatus = MergeStateStatus.BLOCKED
branch_protection.requiresApprovingReviews = True
branch_protection.requiredApprovingReviewCount = 1
review.state = PRReviewState.CHANGES_REQUESTED
review.author.permission = Permission.WRITE

await mergeable(
api=api,
pull_request=pull_request,
branch_protection=branch_protection,
reviews=[review],
)
assert api.set_status.call_count == 1
assert api.dequeue.call_count == 1
assert "changes requested by 'ghost'" in api.set_status.calls[0]["msg"]

# verify we haven't tried to update/merge the PR
assert api.update_branch.called is False
assert api.merge.called is False
assert api.queue_for_merge.called is False


@pytest.mark.asyncio
async def test_mergeable_missing_required_approving_reviews_missing_approving_review_count() -> None:
"""
Don't merge when branch protection requires approving reviews and we don't have enough.
"""
api = create_api()
mergeable = create_mergeable()
pull_request = create_pull_request()
branch_protection = create_branch_protection()
review = create_review()

pull_request.mergeStateStatus = MergeStateStatus.BLOCKED
branch_protection.requiresApprovingReviews = True
branch_protection.requiredApprovingReviewCount = 2
review.state = PRReviewState.APPROVED
review.author.permission = Permission.WRITE

await mergeable(
api=api,
pull_request=pull_request,
branch_protection=branch_protection,
reviews=[review],
)
assert api.set_status.call_count == 1
assert api.dequeue.call_count == 1
assert "missing required reviews, have 1/2" in api.set_status.calls[0]["msg"]

# verify we haven't tried to update/merge the PR
assert api.update_branch.called is False
assert api.merge.called is False
assert api.queue_for_merge.called is False


@pytest.mark.asyncio
async def test_mergeable_missing_required_approving_reviews_code_owners() -> None:
"""
Expand Down Expand Up @@ -662,58 +532,6 @@ async def test_mergeable_wait_for_checks() -> None:
assert api.queue_for_merge.called is False


@pytest.mark.asyncio
async def test_regression_mishandling_multiple_reviews_failing_reviews() -> None:
mergeable = create_mergeable()
api = create_api()
pull_request = create_pull_request()
branch_protection = create_branch_protection()
review_request = create_review_request()
pull_request.mergeStateStatus = MergeStateStatus.BEHIND
branch_protection.requiresApprovingReviews = True
branch_protection.requiredApprovingReviewCount = 2
first_review_date = datetime(2010, 5, 15)
latest_review_date = first_review_date + timedelta(minutes=20)

await mergeable(
api=api,
pull_request=pull_request,
branch_protection=branch_protection,
review_requests=[review_request],
reviews=[
PRReview(
state=PRReviewState.CHANGES_REQUESTED,
createdAt=first_review_date,
author=PRReviewAuthor(login="chdsbd", permission=Permission.WRITE),
),
PRReview(
state=PRReviewState.COMMENTED,
createdAt=latest_review_date,
author=PRReviewAuthor(login="chdsbd", permission=Permission.WRITE),
),
PRReview(
state=PRReviewState.APPROVED,
createdAt=latest_review_date,
author=PRReviewAuthor(login="ghost", permission=Permission.WRITE),
),
PRReview(
state=PRReviewState.APPROVED,
createdAt=latest_review_date,
author=PRReviewAuthor(login="kodiak", permission=Permission.WRITE),
),
],
)
assert api.set_status.call_count == 1
assert "changes requested" in api.set_status.calls[0]["msg"]
assert "chdsbd" in api.set_status.calls[0]["msg"]
assert api.dequeue.call_count == 1

# verify we haven't tried to update/merge the PR
assert api.update_branch.called is False
assert api.merge.called is False
assert api.queue_for_merge.called is False


@pytest.mark.asyncio
async def test_regression_mishandling_multiple_reviews_okay_reviews() -> None:
mergeable = create_mergeable()
Expand Down