Autorerun stale checks on approval (#161)

- Add feature under flag to rerun stale checks if PR is approved
- Add unit tests for new functionality


Pull Request synchronized with [Asana
task](https://app.asana.com/0/0/1206501097102591)

src/config.py

@@ -57,3 +57,6 @@ def is_feature_flag_enabled(flag_name: str) -> bool:
     ).split(",")
     if base_ref
 }
+SGTM_FEATURE__CHECK_RERUN_ON_APPROVAL_ENABLED = is_feature_flag_enabled(
+    "SGTM_FEATURE__CHECK_RERUN_ON_APPROVAL_ENABLED"
+)

src/github/logic.py

@@ -19,6 +19,7 @@
     SGTM_FEATURE__FOLLOWUP_REVIEW_GITHUB_USERS,
     SGTM_FEATURE__CHECK_RERUN_THRESHOLD_HOURS,
     SGTM_FEATURE__CHECK_RERUN_BASE_REF_NAMES,
+    SGTM_FEATURE__CHECK_RERUN_ON_APPROVAL_ENABLED,
 )
 
 GITHUB_MENTION_REGEX = "\B@([a-zA-Z0-9_\-]+)"
@@ -246,17 +247,11 @@ def maybe_add_automerge_warning_comment(pull_request: PullRequest):
 
 # returns True if the pull request was automerged, False if not
 def maybe_automerge_pull_request_and_rerun_stale_checks(
-    pull_request: PullRequest,
+    pull_request: PullRequest, did_rerun_stale_required_checks: bool = False
 ) -> bool:
     is_pull_request_ready_for_automerge = False
     did_rerun_stale_required_checks = False
-    logger.info("Running maybe automerge pull request and rerun stale checks")
-    logger.info(f"Pull request status: {pull_request.to_raw()}")
-    if (
-        not SGTM_FEATURE__AUTOMERGE_ENABLED
-        or pull_request.closed()
-        or pull_request.merged()
-    ):
+    if not SGTM_FEATURE__AUTOMERGE_ENABLED or not _pull_request_is_open(pull_request):
         logger.info(f"Skipping automerge for {pull_request.id()} because it is closed")
         is_pull_request_ready_for_automerge = False
 
@@ -282,10 +277,12 @@ def maybe_automerge_pull_request_and_rerun_stale_checks(
             and pull_request.is_mergeable()
             and pull_request.is_approved()
         )
-        did_rerun_stale_required_checks = (
-            is_pull_request_ready_for_automerge
-            and _maybe_rerun_stale_checks(pull_request)
-        )
+        # if PR has already rerun checks, we don't need to rerun them again
+        if not did_rerun_stale_required_checks:
+            did_rerun_stale_required_checks = (
+                is_pull_request_ready_for_automerge
+                and _maybe_rerun_stale_checks(pull_request)
+            )
     elif pull_request_has_label(pull_request, AutomergeLabel.AFTER_APPROVAL.value):
         is_pull_request_ready_for_automerge = (
             pull_request.is_mergeable() and pull_request.is_approved()
@@ -313,11 +310,33 @@ def maybe_automerge_pull_request_and_rerun_stale_checks(
     return False
 
 
+def maybe_rerun_stale_checks_on_approved_pull_request(
+    pull_request: PullRequest,
+) -> bool:
+    if (
+        SGTM_FEATURE__CHECK_RERUN_ON_APPROVAL_ENABLED
+        and _pull_request_is_open(pull_request)
+        and pull_request.is_approved()
+    ):
+        logger.info(
+            f"PR-{pull_request.id()} is open and approved, maybe rerun stale checks"
+        )
+        return _maybe_rerun_stale_checks(pull_request)
+    logger.info(
+        f"{pull_request.id()} is {'' if _pull_request_is_open(pull_request) else 'not '}open and {'' if pull_request.is_approved() else 'not '}approved"
+    )
+    return False
+
+
 # ----------------------------------------------------------------------------------
 # Automerge helpers
 # ----------------------------------------------------------------------------------
 
 
+def _pull_request_is_open(pull_request: PullRequest) -> bool:
+    return not pull_request.closed() and not pull_request.merged()
+
+
 def _pull_request_has_automerge_comment(
     pull_request: PullRequest, automerge_comment: str
 ) -> bool:

src/github/webhook.py

@@ -16,8 +16,15 @@ def _handle_pull_request_webhook(payload: dict) -> HttpResponse:
     pull_request_id = payload["pull_request"]["node_id"]
     pull_request = graphql_client.get_pull_request(pull_request_id)
     with dynamodb_lock(pull_request_id):
+        pull_request = graphql_client.get_pull_request(pull_request_id)
+        # maybe rerun stale checks on approved PR before attempting to automerge
+        did_rerun_stale_required_checks = (
+            github_logic.maybe_rerun_stale_checks_on_approved_pull_request(pull_request)
+        )
         # a label change will trigger this webhook, so it may trigger automerge
-        github_logic.maybe_automerge_pull_request_and_rerun_stale_checks(pull_request)
+        github_logic.maybe_automerge_pull_request_and_rerun_stale_checks(
+            pull_request, did_rerun_stale_required_checks
+        )
         github_logic.maybe_add_automerge_warning_comment(pull_request)
         github_controller.upsert_pull_request(pull_request)
         return HttpResponse("200")

terraform/main.tf

@@ -185,6 +185,7 @@ resource "aws_lambda_function" "sgtm" {
       SGTM_FEATURE__FOLLOWUP_REVIEW_GITHUB_USERS = var.sgtm_feature__followup_review_github_users,
       SGTM_FEATURE__CHECK_RERUN_THRESHOLD_HOURS = var.sgtm_feature__check_rerun_threshold_hours,
       SGTM_FEATURE__CHECK_RERUN_BASE_REF_NAMES = var.sgtm_feature__check_rerun_base_ref_names,
+      SGTM_FEATURE__CHECK_RERUN_ON_APPROVAL_ENABLED = var.sgtm_feature__check_rerun_on_approval_enabled
     }
   }
 }

terraform/variables.tf

@@ -89,3 +89,9 @@ variable "sgtm_feature__check_rerun_base_ref_names" {
   description = "A comma-separated list of base refs that will trigger check run rerequests when stale. Must be combined with sgtm_feature__check_rerun_freshness_duration_hours."
   default     = "main,master"
 }
+
+variable "sgtm_feature__check_rerun_on_approval_enabled" {
+  type        = string
+  description = "'true' if a check rerun should be rerequested when a PR is approved. Must be combined with sgtm_feature__check_rerun_base_ref_names and sgtm_feature__check_rerun_threshold_hours."
+  default     = "false"
+}

test/github/test_logic.py

@@ -634,6 +634,141 @@ def test_maybe_rerun_stale_checks_multiple_base_refs(
             check_run.database_id(),
         )
 
+    @patch("src.github.logic.SGTM_FEATURE__CHECK_RERUN_ON_APPROVAL_ENABLED", True)
+    @patch("src.github.logic.SGTM_FEATURE__CHECK_RERUN_THRESHOLD_HOURS", 1)
+    @patch(
+        "src.github.logic.SGTM_FEATURE__CHECK_RERUN_BASE_REF_NAMES",
+        ["main"],
+    )
+    def test_rerun_stale_checks_on_approved_pull_request(
+        self, mock_rerequest_check_run
+    ):
+        check_run = build(builder.check_run().completed_at("2020-01-13T14:59:58Z"))
+        pull_request = build(
+            builder.pull_request()
+            .base_ref_name("main")
+            .commit(
+                builder.commit()
+                .status(Commit.BUILD_SUCCESSFUL)
+                .check_suites([builder.check_suite().check_runs([check_run])])
+            )
+            .review(
+                builder.review()
+                .submitted_at("2020-01-13T14:59:58Z")
+                .state(ReviewState.APPROVED)
+            )
+            .merged(False)
+        )
+        self.assertTrue(
+            github_logic.maybe_rerun_stale_checks_on_approved_pull_request(pull_request)
+        )
+        mock_rerequest_check_run.assert_called_once_with(
+            pull_request.repository_owner_handle(),
+            pull_request.repository_name(),
+            check_run.database_id(),
+        )
+
+    @patch("src.github.logic.SGTM_FEATURE__CHECK_RERUN_ON_APPROVAL_ENABLED", True)
+    @patch("src.github.logic.SGTM_FEATURE__CHECK_RERUN_THRESHOLD_HOURS", 1)
+    @patch(
+        "src.github.logic.SGTM_FEATURE__CHECK_RERUN_BASE_REF_NAMES",
+        ["main"],
+    )
+    def test_no_rerun_stale_checks_on_unapproved_pull_request(
+        self, mock_rerequest_check_run
+    ):
+        check_run = build(builder.check_run().completed_at("2020-01-13T14:59:58Z"))
+        pull_request = build(
+            builder.pull_request()
+            .base_ref_name("main")
+            .commit(
+                builder.commit()
+                .status(Commit.BUILD_SUCCESSFUL)
+                .check_suites([builder.check_suite().check_runs([check_run])])
+            )
+            .review(
+                builder.review()
+                .submitted_at("2020-01-13T14:59:58Z")
+                .state(ReviewState.CHANGES_REQUESTED)
+            )
+            .merged(False)
+        )
+        self.assertFalse(
+            github_logic.maybe_rerun_stale_checks_on_approved_pull_request(pull_request)
+        )
+        mock_rerequest_check_run.assert_not_called()
+
+    @patch("src.github.logic.SGTM_FEATURE__CHECK_RERUN_ON_APPROVAL_ENABLED", True)
+    @patch("src.github.logic.SGTM_FEATURE__CHECK_RERUN_THRESHOLD_HOURS", 1)
+    @patch(
+        "src.github.logic.SGTM_FEATURE__CHECK_RERUN_BASE_REF_NAMES",
+        ["main"],
+    )
+    @patch("src.github.logic.SGTM_FEATURE__AUTOMERGE_ENABLED", True)
+    @patch.object(github_client, "merge_pull_request")
+    def test_rerun_stale_checks_on_automerge_pr(
+        self, mock_merge_pull_request, mock_rerequest_check_run
+    ):
+        check_run = build(builder.check_run().completed_at("2020-01-13T14:59:58Z"))
+        pull_request = build(
+            builder.pull_request()
+            .base_ref_name("main")
+            .commit(
+                builder.commit()
+                .status(Commit.BUILD_SUCCESSFUL)
+                .check_suites([builder.check_suite().check_runs([check_run])])
+            )
+            .review(
+                builder.review()
+                .submitted_at("2020-01-13T14:59:58Z")
+                .state(ReviewState.APPROVED)
+            )
+            .label(
+                builder.label().name(
+                    github_logic.AutomergeLabel.AFTER_TESTS_AND_APPROVAL.value
+                )
+            )
+            .merged(False)
+        )
+        self.assertTrue(
+            github_logic.maybe_rerun_stale_checks_on_approved_pull_request(pull_request)
+        )
+        mock_rerequest_check_run.assert_called_once_with(
+            pull_request.repository_owner_handle(),
+            pull_request.repository_name(),
+            check_run.database_id(),
+        )
+
+        self.assertFalse(
+            github_logic.maybe_automerge_pull_request_and_rerun_stale_checks(
+                pull_request
+            )
+        )
+        mock_merge_pull_request.assert_not_called()
+
+    @patch("src.github.logic.SGTM_FEATURE__CHECK_RERUN_ON_APPROVAL_ENABLED", False)
+    def test_noop_if_feature_not_enabled(self, mock_rerequest_check_run):
+        check_run = build(builder.check_run().completed_at("2020-01-13T14:59:58Z"))
+        pull_request = build(
+            builder.pull_request()
+            .base_ref_name("main")
+            .commit(
+                builder.commit()
+                .status(Commit.BUILD_SUCCESSFUL)
+                .check_suites([builder.check_suite().check_runs([check_run])])
+            )
+            .review(
+                builder.review()
+                .submitted_at("2020-01-13T14:59:58Z")
+                .state(ReviewState.APPROVED)
+            )
+            .merged(False)
+        )
+        self.assertFalse(
+            github_logic.maybe_rerun_stale_checks_on_approved_pull_request(pull_request)
+        )
+        mock_rerequest_check_run.assert_not_called()
+
 
 class TestPullRequestHasLabel(unittest.TestCase):
     def test_pull_request_with_label(self):