diff --git a/bot/kodiak/evaluation.py b/bot/kodiak/evaluation.py index 42ee717b7..9762306f8 100644 --- a/bot/kodiak/evaluation.py +++ b/bot/kodiak/evaluation.py @@ -463,7 +463,7 @@ async def mergeable( pull_request: PullRequest, branch_protection: Optional[BranchProtectionRule], review_requests: List[PRReviewRequest], - reviews: List[PRReview], + bot_reviews: List[PRReview], contexts: List[StatusContext], check_runs: List[CheckRun], commits: List[Commit], @@ -657,7 +657,7 @@ async def set_status(msg: str, markdown_content: Optional[str] = None) -> None: ): # if the PR was created by an approve author and we have not previously # given an approval, approve the PR. - sorted_reviews = sorted(reviews, key=lambda x: x.createdAt) + sorted_reviews = sorted(bot_reviews, key=lambda x: x.createdAt) kodiak_reviews = [ review for review in sorted_reviews if review.author.login == KODIAK_LOGIN ] diff --git a/bot/kodiak/pull_request.py b/bot/kodiak/pull_request.py index 394745194..c2e5222eb 100644 --- a/bot/kodiak/pull_request.py +++ b/bot/kodiak/pull_request.py @@ -107,7 +107,7 @@ async def evaluate_pr( pull_request=pr.event.pull_request, branch_protection=pr.event.branch_protection, review_requests=pr.event.review_requests, - reviews=pr.event.reviews, + bot_reviews=pr.event.bot_reviews, contexts=pr.event.status_contexts, check_runs=pr.event.check_runs, commits=pr.event.commits, diff --git a/bot/kodiak/queries/__init__.py b/bot/kodiak/queries/__init__.py index ebb8373b4..8a3020f94 100644 --- a/bot/kodiak/queries/__init__.py +++ b/bot/kodiak/queries/__init__.py @@ -1,11 +1,10 @@ from __future__ import annotations -import asyncio import urllib from dataclasses import dataclass, field from datetime import datetime, timedelta, timezone from enum import Enum -from typing import Any, Dict, List, Mapping, MutableMapping, Optional, Set, Union, cast +from typing import Any, Dict, List, Mapping, MutableMapping, Optional, Union, cast import httpx as http import jwt @@ -160,12 +159,9 @@ def get_event_info_query(requires_conversation_resolution: bool) -> str: name } } - requiresApprovingReviews - requiredApprovingReviewCount requiresStatusChecks requiredStatusCheckContexts requiresStrictStatusChecks - requiresCodeOwnerReviews requiresCommitSignatures %(requiresConversationResolution)s restrictsPushes @@ -394,8 +390,9 @@ class PullRequest(BaseModel): author: PullRequestAuthor isDraft: bool mergeStateStatus: MergeStateStatus - # null if the pull request does not require a review by default (no branch - # protection), and no review was requested or submitted yet. + # null if the pull request does not require a review (no branch protection + # rule). Otherwise shows if the pull request meets the branch protection + # requirements. reviewDecision: Optional[PullRequestReviewDecision] reviewThreads: ReviewThreadConnection state: PullRequestState @@ -428,7 +425,7 @@ class EventInfoResponse: branch_protection: Optional[BranchProtectionRule] review_requests: List[PRReviewRequest] head_exists: bool - reviews: List[PRReview] = field(default_factory=list) + bot_reviews: List[PRReview] = field(default_factory=list) status_contexts: List[StatusContext] = field(default_factory=list) check_runs: List[CheckRun] = field(default_factory=list) valid_merge_methods: List[MergeMethod] = field(default_factory=list) @@ -471,12 +468,9 @@ class BranchProtectionRule(BaseModel): https://developer.github.com/v4/object/branchprotectionrule/ """ - requiresApprovingReviews: bool - requiredApprovingReviewCount: Optional[int] requiresStatusChecks: bool requiredStatusCheckContexts: List[str] requiresStrictStatusChecks: bool - requiresCodeOwnerReviews: bool requiresCommitSignatures: bool requiresConversationResolution: Optional[bool] restrictsPushes: bool @@ -952,24 +946,12 @@ async def get_permissions_for_username(self, username: str) -> Permission: log.exception("get_permissions parse error") return Permission.NONE - # TODO(chdsbd): We may want to cache this response to improve performance as - # we could encounter a lot of throttling when hitting the Github API - async def get_reviewers_and_permissions( - self, *, reviews: List[PRReviewSchema] - ) -> List[PRReview]: - reviewer_names: Set[str] = { - review.author.login - for review in reviews - if review.author and review.author.type != Actor.Bot - } - + async def get_bot_reviews(self, *, reviews: List[PRReviewSchema]) -> List[PRReview]: bot_reviews: List[PRReview] = [] for review in reviews: if not review.author: continue - if review.author.type == Actor.User: - reviewer_names.add(review.author.login) - elif review.author.type == Actor.Bot: + if review.author.type == Actor.Bot: # Bots either have read or write permissions for a pull request, # so if they've been able to write a review on a PR, their # review counts as a user with write access. @@ -983,27 +965,8 @@ async def get_reviewers_and_permissions( ) ) - requests = [ - self.get_permissions_for_username(username) for username in reviewer_names - ] - permissions = await asyncio.gather(*requests) - - user_permission_mapping = dict(zip(reviewer_names, permissions)) - return sorted( - bot_reviews - + [ - PRReview( - state=review.state, - createdAt=review.createdAt, - author=PRReviewAuthor( - login=review.author.login, - permission=user_permission_mapping[review.author.login], - ), - ) - for review in reviews - if review.author and review.author.type == Actor.User - ], + bot_reviews, key=lambda x: x.createdAt, ) @@ -1130,9 +1093,7 @@ async def get_event_info(self, pr_number: int) -> Optional[EventInfoResponse]: ) partial_reviews = get_reviews(pr=pull_request) - reviews_with_permissions = await self.get_reviewers_and_permissions( - reviews=partial_reviews - ) + bot_reviews = await self.get_bot_reviews(reviews=partial_reviews) return EventInfoResponse( config=cfg.parsed, config_str=cfg.text, @@ -1148,7 +1109,7 @@ async def get_event_info(self, pr_number: int) -> Optional[EventInfoResponse]: subscription=subscription, branch_protection=branch_protection, review_requests=get_requested_reviews(pr=pull_request), - reviews=reviews_with_permissions, + bot_reviews=bot_reviews, status_contexts=get_status_contexts(pr=pull_request), commits=get_commits(pr=pull_request), check_runs=get_check_runs(pr=pull_request), diff --git a/bot/kodiak/test_evaluation.py b/bot/kodiak/test_evaluation.py index 41583c3f5..b6b532b6a 100644 --- a/bot/kodiak/test_evaluation.py +++ b/bot/kodiak/test_evaluation.py @@ -257,12 +257,9 @@ def create_pull_request() -> PullRequest: def create_branch_protection() -> BranchProtectionRule: return BranchProtectionRule( - requiresApprovingReviews=True, - requiredApprovingReviewCount=1, requiresStatusChecks=True, requiredStatusCheckContexts=["ci/api"], requiresStrictStatusChecks=True, - requiresCodeOwnerReviews=True, requiresCommitSignatures=False, requiresConversationResolution=False, restrictsPushes=False, @@ -330,7 +327,7 @@ async def __call__( pull_request: PullRequest = ..., branch_protection: Optional[BranchProtectionRule] = ..., review_requests: List[PRReviewRequest] = ..., - reviews: List[PRReview] = ..., + bot_reviews: List[PRReview] = ..., contexts: List[StatusContext] = ..., check_runs: List[CheckRun] = ..., commits: List[Commit] = ..., @@ -358,7 +355,7 @@ async def mergeable( pull_request: PullRequest = create_pull_request(), branch_protection: Optional[BranchProtectionRule] = create_branch_protection(), review_requests: List[PRReviewRequest] = [], - reviews: List[PRReview] = [create_review()], + bot_reviews: List[PRReview] = [create_review()], contexts: List[StatusContext] = [create_context()], check_runs: List[CheckRun] = [create_check_run()], commits: List[Commit] = [], @@ -388,7 +385,7 @@ async def mergeable( pull_request=pull_request, branch_protection=branch_protection, review_requests=review_requests, - reviews=reviews, + bot_reviews=bot_reviews, contexts=contexts, check_runs=check_runs, commits=commits, @@ -1832,7 +1829,7 @@ async def test_mergeable_auto_approve() -> None: pull_request = create_pull_request() config.approve.auto_approve_usernames = ["dependency-updater"] pull_request.author.login = "dependency-updater" - await mergeable(api=api, config=config, pull_request=pull_request, reviews=[]) + await mergeable(api=api, config=config, pull_request=pull_request, bot_reviews=[]) assert api.approve_pull_request.call_count == 1 assert api.set_status.call_count == 1 assert "enqueued for merge (position=4th)" in api.set_status.calls[0]["msg"] @@ -1858,7 +1855,9 @@ async def test_mergeable_auto_approve_existing_approval() -> None: pull_request.author.login = "dependency-updater" review.author.login = "kodiak-test-app" review.state = PRReviewState.APPROVED - await mergeable(api=api, config=config, pull_request=pull_request, reviews=[review]) + await mergeable( + api=api, config=config, pull_request=pull_request, bot_reviews=[review] + ) assert api.approve_pull_request.call_count == 0 assert api.set_status.call_count == 1 assert "enqueued for merge (position=4th)" in api.set_status.calls[0]["msg"] @@ -1884,7 +1883,9 @@ async def test_mergeable_auto_approve_old_approval() -> None: pull_request.author.login = "dependency-updater" review.author.login = "kodiak-test-app" review.state = PRReviewState.DISMISSED - await mergeable(api=api, config=config, pull_request=pull_request, reviews=[review]) + await mergeable( + api=api, config=config, pull_request=pull_request, bot_reviews=[review] + ) assert api.approve_pull_request.call_count == 1 assert api.set_status.call_count == 1 assert "enqueued for merge (position=4th)" in api.set_status.calls[0]["msg"] @@ -1909,7 +1910,9 @@ async def test_mergeable_auto_approve_ignore_closed_merged_prs() -> None: config.approve.auto_approve_usernames = ["dependency-updater"] pull_request.author.login = "dependency-updater" pull_request.state = pull_request_state - await mergeable(api=api, config=config, pull_request=pull_request, reviews=[]) + await mergeable( + api=api, config=config, pull_request=pull_request, bot_reviews=[] + ) assert api.approve_pull_request.call_count == 0 assert api.set_status.call_count == 0 assert api.queue_for_merge.call_count == 0 @@ -1944,7 +1947,9 @@ async def test_mergeable_auto_approve_ignore_draft_pr() -> None: pull_request_via_merge_state_status, ): api = create_api() - await mergeable(api=api, config=config, pull_request=pull_request, reviews=[]) + await mergeable( + api=api, config=config, pull_request=pull_request, bot_reviews=[] + ) assert api.approve_pull_request.call_count == 0 assert api.set_status.call_count == 1 assert ( diff --git a/bot/kodiak/test_pull_request.py b/bot/kodiak/test_pull_request.py index d50c55b6c..f26a947d8 100644 --- a/bot/kodiak/test_pull_request.py +++ b/bot/kodiak/test_pull_request.py @@ -56,15 +56,12 @@ def create_event() -> EventInfoResponse: delete_branch_on_merge=False, ) branch_protection = BranchProtectionRule( - requiresApprovingReviews=True, - requiredApprovingReviewCount=2, requiresStatusChecks=True, requiredStatusCheckContexts=[ "ci/circleci: frontend_lint", "ci/circleci: frontend_test", ], requiresStrictStatusChecks=True, - requiresCodeOwnerReviews=False, requiresCommitSignatures=False, requiresConversationResolution=False, restrictsPushes=False, @@ -84,7 +81,7 @@ def create_event() -> EventInfoResponse: repository=rep_info, branch_protection=branch_protection, review_requests=[], - reviews=[], + bot_reviews=[], status_contexts=[], check_runs=[], valid_merge_methods=[MergeMethod.squash], diff --git a/bot/kodiak/test_queries.py b/bot/kodiak/test_queries.py index 5539b8908..465c29bed 100644 --- a/bot/kodiak/test_queries.py +++ b/bot/kodiak/test_queries.py @@ -11,7 +11,6 @@ from kodiak import app_config as conf from kodiak.config import V1, Merge, MergeMethod from kodiak.queries import ( - Actor, BranchProtectionRule, CheckConclusionState, CheckRun, @@ -25,9 +24,7 @@ Permission, PRReview, PRReviewAuthor, - PRReviewAuthorSchema, PRReviewRequest, - PRReviewSchema, PRReviewState, PullRequest, PullRequestAuthor, @@ -161,8 +158,6 @@ def block_event() -> EventInfoResponse: is_private=True, ) branch_protection = BranchProtectionRule( - requiresApprovingReviews=True, - requiredApprovingReviewCount=2, requiresStatusChecks=True, requiredStatusCheckContexts=[ "ci/circleci: backend_lint", @@ -172,7 +167,6 @@ def block_event() -> EventInfoResponse: "WIP (beta)", ], requiresStrictStatusChecks=True, - requiresCodeOwnerReviews=False, requiresCommitSignatures=False, requiresConversationResolution=False, restrictsPushes=True, @@ -204,32 +198,7 @@ def block_event() -> EventInfoResponse: PRReviewRequest(name="ghost-team"), PRReviewRequest(name="ghost-mannequin"), ], - reviews=[ - PRReview( - createdAt=datetime.fromisoformat("2019-05-22T15:29:34+00:00"), - state=PRReviewState.COMMENTED, - author=PRReviewAuthor(login="ghost", permission=Permission.WRITE), - ), - PRReview( - createdAt=datetime.fromisoformat("2019-05-22T15:29:52+00:00"), - state=PRReviewState.CHANGES_REQUESTED, - author=PRReviewAuthor(login="ghost", permission=Permission.WRITE), - ), - PRReview( - createdAt=datetime.fromisoformat("2019-05-22T15:30:52+00:00"), - state=PRReviewState.COMMENTED, - author=PRReviewAuthor(login="kodiak", permission=Permission.ADMIN), - ), - PRReview( - createdAt=datetime.fromisoformat("2019-05-22T15:43:17+00:00"), - state=PRReviewState.APPROVED, - author=PRReviewAuthor(login="ghost", permission=Permission.WRITE), - ), - PRReview( - createdAt=datetime.fromisoformat("2019-05-23T15:13:29+00:00"), - state=PRReviewState.APPROVED, - author=PRReviewAuthor(login="walrus", permission=Permission.WRITE), - ), + bot_reviews=[ PRReview( createdAt=datetime.fromisoformat("2019-05-24T10:21:32+00:00"), state=PRReviewState.APPROVED, @@ -771,48 +740,6 @@ def test_get_commits_error_handling_missing_response() -> None: assert res == [] -@pytest.mark.asyncio -async def test_get_reviewers_and_permissions_empty_author( - mocker: MockFixture, api_client: Client -) -> None: - """ - We should ignore reviews with missing authors. - - `author` becomes null if the GitHub user's account is deleted. This is shown - in the GitHub UI as the "ghost" user. - """ - - async def get_permissions_for_username_patch(username: str) -> Permission: - if username == "jdoe": - return Permission.WRITE - raise Exception - - mocker.patch.object( - api_client, "get_permissions_for_username", get_permissions_for_username_patch - ) - res = await api_client.get_reviewers_and_permissions( - reviews=[ - PRReviewSchema( - createdAt=datetime.fromisoformat("2019-05-22T15:29:34+00:00"), - state=PRReviewState.COMMENTED, - author=PRReviewAuthorSchema(login="jdoe", type=Actor.User), - ), - PRReviewSchema( - createdAt=datetime.fromisoformat("2019-05-22T15:29:52+00:00"), - state=PRReviewState.APPROVED, - author=None, - ), - ] - ) - assert res == [ - PRReview( - createdAt=datetime.fromisoformat("2019-05-22T15:29:34+00:00"), - state=PRReviewState.COMMENTED, - author=PRReviewAuthor(login="jdoe", permission=Permission.WRITE), - ) - ] - - def generate_page_of_prs(numbers: Iterable[int]) -> Response: """ Create a fake page for the list-pull-requests API. diff --git a/bot/kodiak/tests/evaluation/test_branch_protection.py b/bot/kodiak/tests/evaluation/test_branch_protection.py index b1cda95ac..5cbff7a1c 100644 --- a/bot/kodiak/tests/evaluation/test_branch_protection.py +++ b/bot/kodiak/tests/evaluation/test_branch_protection.py @@ -1,5 +1,3 @@ -from datetime import datetime, timedelta - import pytest from kodiak.config import MergeMethod @@ -8,10 +6,6 @@ CheckConclusionState, MergeStateStatus, NodeListPushAllowance, - Permission, - PRReview, - PRReviewAuthor, - PRReviewState, PullRequestReviewDecision, PushAllowance, PushAllowanceActor, @@ -26,8 +20,6 @@ create_context, create_mergeable, create_pull_request, - create_review, - create_review_request, ) @@ -189,15 +181,13 @@ async def test_mergeable_requires_commit_signatures_squash_and_merge() -> None: @pytest.mark.asyncio -async def test_mergeable_missing_required_approving_reviews_code_owners() -> None: +async def test_mergeable_missing_required_review() -> None: """ - Don't merge when code owners are required for review. + Don't merge when a review is required. """ api = create_api() mergeable = create_mergeable() pull_request = create_pull_request() - branch_protection = create_branch_protection() - review = create_review() pull_request.mergeStateStatus = MergeStateStatus.BLOCKED pull_request.reviewDecision = PullRequestReviewDecision.REVIEW_REQUIRED @@ -205,19 +195,9 @@ async def test_mergeable_missing_required_approving_reviews_code_owners() -> Non # request, even if the pull request was blocked from merging. pull_request.mergeStateStatus = MergeStateStatus.BEHIND - branch_protection.requiresApprovingReviews = True - branch_protection.requiredApprovingReviewCount = 1 - branch_protection.requiresCodeOwnerReviews = True - # this pull request meets requiredApprovingReviewCount, but is missing a - # code owner approval. - 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 @@ -532,144 +512,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_okay_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 = 1 - 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="chdsbd", permission=Permission.WRITE), - ), - PRReview( - state=PRReviewState.APPROVED, - createdAt=latest_review_date, - author=PRReviewAuthor(login="ghost", permission=Permission.WRITE), - ), - ], - ) - assert api.set_status.call_count == 1 - assert "enqueued for merge (position=" in api.set_status.calls[0]["msg"] - assert api.dequeue.call_count == 0 - assert api.queue_for_merge.call_count == 1 - - # verify we haven't tried to update/merge the PR - assert api.merge.called is False - assert api.update_branch.called is False - - -@pytest.mark.asyncio -async def test_regression_mishandling_multiple_reviews_okay_dismissed_reviews() -> None: - mergeable = create_mergeable() - pull_request = create_pull_request() - branch_protection = create_branch_protection() - review_request = create_review_request() - api = create_api() - pull_request.mergeStateStatus = MergeStateStatus.BEHIND - branch_protection.requiresApprovingReviews = True - branch_protection.requiredApprovingReviewCount = 1 - 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.DISMISSED, - 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), - ), - ], - ) - assert api.set_status.call_count == 1 - assert "enqueued for merge (position=" in api.set_status.calls[0]["msg"] - assert api.dequeue.call_count == 0 - assert api.queue_for_merge.call_count == 1 - - # verify we haven't tried to update/merge the PR - assert api.merge.called is False - assert api.update_branch.called is False - - -@pytest.mark.asyncio -async def test_regression_mishandling_multiple_reviews_okay_non_member_reviews() -> None: - mergeable = create_mergeable() - pull_request = create_pull_request() - branch_protection = create_branch_protection() - review_request = create_review_request() - api = create_api() - pull_request.mergeStateStatus = MergeStateStatus.BEHIND - branch_protection.requiresApprovingReviews = True - branch_protection.requiredApprovingReviewCount = 1 - 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.NONE), - ), - PRReview( - state=PRReviewState.APPROVED, - createdAt=latest_review_date, - author=PRReviewAuthor(login="ghost", permission=Permission.WRITE), - ), - ], - ) - assert api.set_status.call_count == 1 - assert "enqueued for merge (position=" in api.set_status.calls[0]["msg"] - assert api.dequeue.call_count == 0 - assert api.queue_for_merge.call_count == 1 - - # verify we haven't tried to update/merge the PR - assert api.merge.called is False - assert api.update_branch.called is False - - @pytest.mark.asyncio async def test_mergeable_do_not_merge_with_update_branch_immediately_waiting_for_checks() -> None: """