better support codeowner reviews (#849)

I'll make another PR to block merge for this case once we log this for a bit.
chdsbd · Oct 26, 2022 · 6a648b0 · 6a648b0
1 parent a11f216
commit 6a648b0
Show file tree

Hide file tree

Showing 6 changed files with 66 additions and 1 deletion.
diff --git a/bot/kodiak/evaluation.py b/bot/kodiak/evaluation.py
@@ -1056,12 +1056,19 @@ async def set_status(msg: str, markdown_content: Optional[str] = None) -> None:
         #
         # We trigger an ApiCallException to trigger the retry handling. Usually
         if not (wait_for_checks or need_branch_update):
+            codeowner_review_required = (
+                pull_request.mergeStateStatus == MergeStateStatus.BLOCKED
+                and pull_request.reviewDecision is None
+                and any(x.asCodeOwner for x in review_requests)
+            )
             log.info(
                 "merge blocked for unknown reason",
                 merging=merging,
                 wait_for_checks=wait_for_checks,
                 need_branch_update=need_branch_update,
+                codeowner_review_required=codeowner_review_required,
             )
+
             if merging:
                 # maybe we want to convert this to be a PollForever exception, but I'm not
                 # confident enough that we'll always want to poll in this scenario.

diff --git a/bot/kodiak/queries/__init__.py b/bot/kodiak/queries/__init__.py
@@ -205,6 +205,7 @@ def get_event_info_query(
       isCrossRepository
       reviewRequests(first: 100) {
         nodes {
+          asCodeOwner
           requestedReviewer {
             __typename
             ... on User {
@@ -527,6 +528,7 @@ class PRReview:
 @dataclass
 class PRReviewRequest:
     name: str
+    asCodeOwner: bool = False
 
 
 class StatusState(Enum):
@@ -642,6 +644,7 @@ def get_requested_reviews(*, pr: Dict[str, Any]) -> List[PRReviewRequest]:
     review_requests: List[PRReviewRequest] = []
     for request_dict in get_review_requests_dicts(pr=pr):
         try:
+            asCodeOwner = request_dict["asCodeOwner"]
             request = request_dict["requestedReviewer"]
             if request is None:
                 continue
@@ -650,7 +653,9 @@ def get_requested_reviews(*, pr: Dict[str, Any]) -> List[PRReviewRequest]:
                 name = request["login"]
             else:
                 name = request["name"]
-            review_requests.append(PRReviewRequest(name=name))
+            review_requests.append(
+                PRReviewRequest(name=name, asCodeOwner=bool(asCodeOwner))
+            )
         except ValueError:
             logger.warning("Could not parse PRReviewRequest", exc_info=True)
     return review_requests

diff --git a/bot/kodiak/test/fixtures/api/get_event/behind.json b/bot/kodiak/test/fixtures/api/get_event/behind.json
@@ -59,18 +59,21 @@
         "reviewRequests": {
           "nodes": [
             {
+              "asCodeOwner": false,
               "requestedReviewer": {
                 "__typename": "User",
                 "login": "ghost"
               }
             },
             {
+              "asCodeOwner": false,
               "requestedReviewer": {
                 "__typename": "Team",
                 "name": "ghost-team"
               }
             },
             {
+              "asCodeOwner": false,
               "requestedReviewer": {
                 "__typename": "Mannequin",
                 "login": "ghost-mannequin"

diff --git a/bot/kodiak/test/fixtures/api/get_event/no_author.json b/bot/kodiak/test/fixtures/api/get_event/no_author.json
@@ -57,24 +57,28 @@
         "reviewRequests": {
           "nodes": [
             {
+              "asCodeOwner": false,
               "requestedReviewer": {
                 "__typename": "User",
                 "login": "ghost"
               }
             },
             {
+              "asCodeOwner": false,
               "requestedReviewer": {
                 "__typename": "Team",
                 "name": "ghost-team"
               }
             },
             {
+              "asCodeOwner": false,
               "requestedReviewer": {
                 "__typename": "Mannequin",
                 "login": "ghost-mannequin"
               }
             },
             {
+              "asCodeOwner": false,
               "requestedReviewer": null
             }
           ]

diff --git a/bot/kodiak/test/fixtures/api/get_event/no_latest_sha.json b/bot/kodiak/test/fixtures/api/get_event/no_latest_sha.json
@@ -61,18 +61,21 @@
                 "reviewRequests": {
                     "nodes": [
                         {
+                            "asCodeOwner": false,
                             "requestedReviewer": {
                                 "__typename": "User",
                                 "login": "ghost"
                             }
                         },
                         {
+                            "asCodeOwner": false,
                             "requestedReviewer": {
                                 "__typename": "Team",
                                 "name": "ghost-team"
                             }
                         },
                         {
+                            "asCodeOwner": false,
                             "requestedReviewer": {
                                 "__typename": "Mannequin",
                                 "login": "ghost-mannequin"

diff --git a/bot/kodiak/test_evaluation.py b/bot/kodiak/test_evaluation.py
@@ -1538,6 +1538,49 @@ async def test_mergeable_unknown_merge_blockage() -> None:
     assert api.queue_for_merge.called is False
 
 
+@pytest.mark.xfail
+@pytest.mark.asyncio
+async def test_mergeable_unknown_merge_blockage_code_owner() -> None:
+    mergeable = create_mergeable()
+    api = create_api()
+    pull_request = create_pull_request()
+    pull_request.mergeStateStatus = MergeStateStatus.BLOCKED
+    pull_request.reviewDecision = None
+
+    await mergeable(
+        api=api,
+        pull_request=pull_request,
+        review_requests=[PRReviewRequest(name="chdsbd", asCodeOwner=True)],
+    )
+
+    assert api.set_status.call_count == 1
+    assert "Codeowner review required" in api.set_status.calls[0]["msg"]
+    assert api.dequeue.call_count == 1
+
+    assert api.update_branch.called is False
+    assert api.requeue.called is False
+    assert api.merge.called is False
+    assert api.queue_for_merge.called is False
+
+
+@pytest.mark.asyncio
+async def test_mergeable_unknown_merge_blockage_code_owner_approval() -> None:
+    mergeable = create_mergeable()
+    api = create_api()
+    pull_request = create_pull_request()
+    pull_request.mergeStateStatus = MergeStateStatus.CLEAN
+    pull_request.reviewDecision = None
+
+    await mergeable(
+        api=api,
+        pull_request=pull_request,
+        review_requests=[PRReviewRequest(name="sbdchd", asCodeOwner=True)],
+    )
+
+    assert api.queue_for_merge.called is True
+    assert api.dequeue.call_count == 0
+
+
 @pytest.mark.asyncio
 async def test_mergeable_prioritize_ready_to_merge() -> None:
     """