diff --git a/publishthing/apps/prgate/github.py b/publishthing/apps/prgate/github.py index 6c3210b..f41301b 100644 --- a/publishthing/apps/prgate/github.py +++ b/publishthing/apps/prgate/github.py @@ -73,7 +73,6 @@ def gate_pull_request( pull_request = event.json_data["pull_request"] number = str(event.json_data["number"]) sender = event.json_data["sender"]["login"] - sha = pull_request["head"]["sha"] gh_repo = thing.github_repo(event.repo_name) @@ -132,23 +131,22 @@ def holds_claim(issue_number: int) -> bool: message = messages.close_message( result, label, - sha, review_label=review_label, deny_label=deny_label, policy_url=entry.get("policy_url"), ) # github redelivers webhooks, and a reopen re-runs the gate; only - # skip the comment when we've already said this exact thing about - # this exact commit. the close below still runs either way, and + # skip the comment when we've already said this exact thing on + # this pull request. the close below still runs either way, and # closing an already-closed pull request is a no-op. - if _already_commented(gh_repo, number, result.reason, sha): + if _already_commented(gh_repo, number, result.reason): thing.debug( "prgate", - "already commented on %s #%s for sha %s, not repeating", + "already commented on %s #%s for %s, not repeating", event.repo_name, number, - sha, + result.reason, ) else: # comment before closing: if the close fails we'd rather have @@ -162,9 +160,9 @@ def holds_claim(issue_number: int) -> bool: def _already_commented( - gh_repo: github.GithubRepo, number: str, reason: str, sha: str + gh_repo: github.GithubRepo, number: str, reason: str ) -> bool: - marker = messages.marker(reason, sha) + marker = messages.marker_match(reason) for comment in gh_repo.get_issue_comments(number): if marker in (comment.get("body") or ""): return True diff --git a/publishthing/apps/prgate/messages.py b/publishthing/apps/prgate/messages.py index 7ad54e7..a7e94c7 100644 --- a/publishthing/apps/prgate/messages.py +++ b/publishthing/apps/prgate/messages.py @@ -17,7 +17,11 @@ from . import util # hidden marker so the gate can recognize its own previous comment on a -# pull request and not repeat itself if github redelivers a webhook. +# pull request and not repeat itself if github redelivers a webhook or +# the pull request is closed and reopened. keyed on the reason alone: +# the gate's decision is a function of the pull request's description and +# the issue it names, never of the code, so a new head commit can't +# change the outcome without also changing the reason. MARKER_PREFIX = "" % (MARKER_PREFIX, reason, sha) +def marker(reason: str) -> str: + return "%s:%s -->" % (MARKER_PREFIX, reason) + + +def marker_match(reason: str) -> str: + """The substring that recognizes any past comment for ``reason``. + + Deliberately unterminated: the marker used to carry the head sha as + well, and matching on the open prefix means the comments already + posted with the old format are still recognized. + + """ + + return "%s:%s" % (MARKER_PREFIX, reason) def claim_marker(issue: int) -> str: @@ -132,7 +148,6 @@ def accepted_message(issue: int, label: str, review_label: str) -> str: def close_message( result: util.GateResult, label: str, - sha: str, review_label: str = util.DEFAULT_REVIEW_LABEL, deny_label: str = util.DEFAULT_DENY_LABEL, policy_url: Optional[str] = None, @@ -162,7 +177,7 @@ def close_message( steps = _STEPS_NO_ISSUE paragraphs = [ - marker(result.reason, sha), + marker(result.reason), _INTRO, _POLICY % subs, (steps % subs).strip(), diff --git a/tests/test_prgate_hook.py b/tests/test_prgate_hook.py index d58ed25..666218e 100644 --- a/tests/test_prgate_hook.py +++ b/tests/test_prgate_hook.py @@ -304,13 +304,27 @@ def test_redelivery_does_not_repeat_the_comment(): assert gh_repo.closed == ["7", "7"] -def test_new_commit_gets_a_fresh_comment(): +def test_new_commit_does_not_get_a_fresh_comment(): + """A reopen after a push is the same rejection, not a new one. + + The marker used to carry the head sha, so pushing a commit and + reopening produced a second identical comment. + + """ + + gh_repo = FakeRepo(comments=[messages.marker(util.CLOSE_NO_ISSUE)]) + run(gh_repo, make_event(body="no reference")) + + assert len(gh_repo.comments) == 1 + + +def test_comments_from_the_old_sha_bearing_marker_are_recognized(): gh_repo = FakeRepo( - comments=[messages.marker(util.CLOSE_NO_ISSUE, "different-sha")] + comments=["" % util.CLOSE_NO_ISSUE] ) run(gh_repo, make_event(body="no reference")) - assert len(gh_repo.comments) == 2 + assert len(gh_repo.comments) == 1 def test_close_can_be_disabled(): @@ -335,19 +349,21 @@ def test_maintainer_exemption_is_per_repo(): assert gh_repo.closed == ["7"] -def test_marker_distinguishes_reason_and_sha(): - assert messages.marker("no_issue", "aaa") != messages.marker( - "no_issue", "bbb" - ) - assert messages.marker("no_issue", "aaa") != messages.marker( - "issue_closed", "aaa" - ) +def test_marker_distinguishes_reason(): + assert messages.marker("no_issue") != messages.marker("issue_closed") def test_already_commented_matches_only_its_own_marker(): gh_repo = FakeRepo(comments=["a normal human comment mentioning #5"]) assert not prgate_github._already_commented( - gh_repo, "7", util.CLOSE_NO_ISSUE, SHA + gh_repo, "7", util.CLOSE_NO_ISSUE + ) + + +def test_already_commented_does_not_match_a_different_reason(): + gh_repo = FakeRepo(comments=[messages.marker(util.CLOSE_ISSUE_CLOSED)]) + assert not prgate_github._already_commented( + gh_repo, "7", util.CLOSE_NO_ISSUE )