diff --git a/src/config.py b/src/config.py index 2b0cd2b2..30241c4e 100644 --- a/src/config.py +++ b/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" +) diff --git a/src/github/logic.py b/src/github/logic.py index a4cb2327..d6b64034 100644 --- a/src/github/logic.py +++ b/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: diff --git a/src/github/webhook.py b/src/github/webhook.py index a053c9ff..20241648 100644 --- a/src/github/webhook.py +++ b/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") diff --git a/terraform/main.tf b/terraform/main.tf index e113dc36..bc26fd50 100644 --- a/terraform/main.tf +++ b/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 } } } diff --git a/terraform/variables.tf b/terraform/variables.tf index 5be46c2c..adede93d 100644 --- a/terraform/variables.tf +++ b/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" +} diff --git a/test/github/test_logic.py b/test/github/test_logic.py index c6650bc4..111123d1 100644 --- a/test/github/test_logic.py +++ b/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):