diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 08ab0dcf..9a0f50c8 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -533,11 +533,16 @@ jobs: pending-label: '' enabled: ${{ inputs.enable_comment && 'true' || 'false' }} + # Nothing to request when the approval already stands. Without this the + # run announces "needs a decision", adds the pending label, and then the + # verdict takes both back seconds later — a flicker in the timeline for a + # decision that was never in question. - name: Request the approval on the pull request if: >- needs.detect.result == 'success' && needs.detect.outputs.in-scope == 'true' && needs.detect.outputs.has-breaking-changes == 'true' + && steps.prior.outputs.already-approved != 'true' && !inputs.dry_run uses: LerianStudio/github-actions-shared-workflows/src/notify/breaking-change-approval-reporter@develop with: diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index b21f6e18..986b7979 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -181,6 +181,23 @@ runs: ) fi + # `carol` -> `[@carol](https://github.com/carol)`. Linked, not a bare + # mention: the person being named is the one who just acted, so a + # notification tells them what they already know. Same reasoning as the + # reviewer list above. + link_people() { + local out="" person + while IFS= read -r person; do + person=$(printf '%s' "$person" | tr -d ' ') + [ -z "$person" ] && continue + out="${out:+$out, }[@${person}](${SERVER_URL}/${person})" + done < <(tr ',' '\n' <<< "$1") + printf '%s' "$out" + } + + linked_approvers=$(link_people "$APPROVERS") + linked_rejecters=$(link_people "$REJECTERS") + commit_link="" if [ -n "$HEAD_SHA" ]; then commit_link="[\`${HEAD_SHA:0:7}\`](${SERVER_URL}/${REPO}/commit/${HEAD_SHA})" @@ -235,14 +252,14 @@ runs: printf '> [!NOTE]\n' printf '> ## ✅ Breaking change approved\n' printf '>\n' - printf '> Approved by **%s**. 💥 The next release will be a major version.\n\n' \ - "${APPROVERS:-a reviewer}" + printf '> Approved by %s. 💥 The next release will be a major version.\n\n' \ + "${linked_approvers:-a reviewer}" ;; rejected) printf '> [!CAUTION]\n' printf '> ## ⛔ Breaking change rejected\n' printf '>\n' - printf '> Rejected by **%s**' "${REJECTERS:-a reviewer}" + printf '> Rejected by %s' "${linked_rejecters:-a reviewer}" [ -n "$REJECTION_COMMENT" ] && printf ' — *%s*' "$REJECTION_COMMENT" printf '.\n' if [ "$CLOSE_ON_REJECTION" = "true" ]; then diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 23c8255f..ff7e8bdb 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -1069,13 +1069,18 @@ def test_the_waiting_comment_links_instead_of_mentioning(self): self.assertIn(fragment, rendered) # The pending deployment already notifies the reviewers; a bare @mention - # would ping the whole team again for the same thing. Checked on the - # rendered comment, so it holds however the composite spells it. - for match in re.finditer(r"@LerianStudio/", rendered): - self.assertTrue( - rendered[: match.start()].endswith("["), - f"bare team mention: ...{rendered[max(0, match.start() - 40):match.end() + 30]}...", - ) + # would ping the whole team again for the same thing. + # + # Same technique as the decider test: delete the complete links, then + # require nothing to remain. This assertion used to check only that an + # `@` was preceded by `[`, which a bare `[@LerianStudio/devops]` with no + # target satisfies. + without_links = re.sub( + r"\[@LerianStudio/\w+\]\(https://github\.com/orgs/LerianStudio/teams/\w+\)", + "", + rendered, + ) + self.assertNotIn("@LerianStudio/", without_links) def test_only_one_approval_request_stays_open_per_pull_request(self): job = extract_job(APPROVAL_WORKFLOW, "approval") @@ -1122,6 +1127,37 @@ def test_a_cancelled_approval_stops_before_anything_can_act_on_it(self): self.assertEqual("superseded", outputs["state"]) self.assertNotEqual("rejected", outputs["state"]) + def test_a_standing_approval_skips_the_request_entirely(self): + step = extract_step( + extract_job(APPROVAL_WORKFLOW, "request"), + "Request the approval on the pull request", + ) + # Observed on #920: with the approval already standing, the run still + # announced "needs a decision" and added the pending label, and the + # verdict took both back seconds later. A flicker in the timeline for a + # decision that was never in question. + self.assertIn("steps.prior.outputs.already-approved != 'true'", step) + + def test_the_decider_is_linked_not_mentioned(self): + for state, env, who in ( + ("approved", {"APPROVERS": "carol"}, "carol"), + ("rejected", {"REJECTERS": "dave"}, "dave"), + ): + with self.subTest(state=state): + rendered = render_reporter(state, **env) + self.assertIn(f"[@{who}](https://github.com/{who})", rendered) + + # Linked, not mentioned: the person named is the one who just + # acted, so a notification tells them what they already know. + # + # Checked by deleting the complete links and requiring nothing + # to remain. "preceded by `[`" would accept a bare `[@carol]` + # with no target — present, bracketed, and not a link. + without_links = re.sub( + rf"\[@{who}\]\(https://github\.com/{who}\)", "", rendered + ) + self.assertNotIn(f"@{who}", without_links) + def test_an_approval_already_given_is_not_asked_for_again(self): # The reviewer said yes to a breaking change. A push that leaves that # change alone must not put the same question back in their queue —