ref(bot): cleanup old permission checks for reviews

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
         ]

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,

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),

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 (

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],

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.