diff --git a/.github/labels.yml b/.github/labels.yml index 8498552c..6e04c717 100644 --- a/.github/labels.yml +++ b/.github/labels.yml @@ -82,6 +82,10 @@ color: "b60205" description: Callers must update their workflow configuration after this change +- name: awaiting-breaking-approval + color: "fbca04" + description: A breaking change on this PR is waiting on a human decision + # ── Capabilities ────────────────────────────────────────────────────────────── - name: build diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 25cf40e2..08ab0dcf 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -144,10 +144,27 @@ on: type: boolean default: true label: - description: Label applied while the pull request waits for its approval, and removed once resolved. Cosmetic — failure to apply it never fails the job. Empty disables it. + description: >- + Label stating that this pull request carries a breaking change. A fact + about the change, so it survives the approval — the break does not go + away because somebody agreed to it. Removed only when the breaking + commits are gone. Empty disables it. + + Behaviour change: this label used to come off on approval. A caller + that set it as a temporary waiting marker should move that value to + `pending_label` and set this one to an empty string. required: false type: string default: breaking-change + pending_label: + description: >- + Label stating that the breaking change is waiting on a decision. A + state, so it comes off once approved or rejected. Separate from + `label` because the two stop being true at different moments. + Empty disables it. + required: false + type: string + default: awaiting-breaking-approval enable_comment: description: Post (and keep updated) a single comment carrying the request, the approval link and the outcome. required: false @@ -512,7 +529,8 @@ jobs: head-sha: ${{ needs.detect.outputs.head-sha }} acknowledged: ${{ needs.detect.outputs.acknowledged }} author-acknowledgement: ${{ inputs.author_acknowledgement }} - label: '' + label: ${{ inputs.label }} + pending-label: '' enabled: ${{ inputs.enable_comment && 'true' || 'false' }} - name: Request the approval on the pull request @@ -536,6 +554,7 @@ jobs: run-url: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} close-on-rejection: ${{ inputs.close_on_rejection && 'true' || 'false' }} label: ${{ inputs.label }} + pending-label: ${{ inputs.pending_label }} enabled: ${{ inputs.enable_comment && 'true' || 'false' }} # ----------------- The gate itself ----------------- @@ -625,6 +644,8 @@ jobs: APPROVERS: ${{ steps.decisions.outputs.approvers }} REJECTERS: ${{ steps.decisions.outputs.rejecters }} API_READABLE: ${{ steps.decisions.outputs.api-readable }} + LABEL: ${{ inputs.label }} + PENDING_LABEL: ${{ inputs.pending_label }} DRY_RUN: ${{ inputs.dry_run && 'true' || 'false' }} run: | set -euo pipefail @@ -705,6 +726,18 @@ jobs: result=success state=none emit + + # The only moment the break actually stopped existing, so the only + # place the fact label comes off. A reword dropping the `!` left + # `breaking-change` on the pull request for good — the cleanup was + # lost in an earlier rewrite of this job and is restored here. + if [ "$DRY_RUN" != "true" ] && [ -n "${PR_NUMBER:-}" ]; then + for stale in "$LABEL" "$PENDING_LABEL"; do + [ -n "$stale" ] || continue + gh pr edit "$PR_NUMBER" --repo "$REPO" --remove-label "$stale" >/dev/null 2>&1 || true + done + fi + echo "::notice::Nothing to approve on PR #${PR_NUMBER} — ${REASON}" exit 0 fi @@ -763,6 +796,7 @@ jobs: rejection-comment: ${{ steps.decisions.outputs.rejection-comment }} close-on-rejection: ${{ inputs.close_on_rejection && 'true' || 'false' }} label: ${{ inputs.label }} + pending-label: ${{ inputs.pending_label }} enabled: ${{ inputs.enable_comment && 'true' || 'false' }} # Only on a recorded rejection, never on a cancellation. Closing a pull diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index be87e051..38e5aebb 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -408,8 +408,24 @@ ends as the record of who decided what: > 👉 **[Review the pending deployment](#)** — *Review deployments* → > `breaking-approval`. -The `breaking-change` label is applied while it waits and removed once resolved — -cosmetic, and a repository that never declared it still gets its gate. +Two labels, and they expire at different times. `breaking-change` states a fact +about the commits and **survives the approval** — the break does not stop being a +break because somebody agreed to it, and a search for that label months later +should still find this pull request. It comes off only when the breaking commits +are gone. `awaiting-breaking-approval` states that a decision is pending, and +comes off the moment one is made. + +Both are cosmetic: a repository that never declared them still gets its gate. +Declare them with [`labels-sync`](labels-sync.md), or pass `label: ''` / +`pending_label: ''` to turn them off. + +> **Behaviour change.** `label` used to be removed once the change was approved +> or rejected. If you set it as a temporary waiting marker, move that value to +> `pending_label` and set `label: ''` — otherwise the marker now stays on the +> pull request after the decision. +> +> Callers on the default need nothing: `breaking-change` staying put is the +> point, and `awaiting-breaking-approval` takes over the transient role. ## Inputs diff --git a/src/notify/breaking-change-approval-reporter/README.md b/src/notify/breaking-change-approval-reporter/README.md index 4af911ef..0d3cc91c 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -82,6 +82,21 @@ may be gated by different people than the caller configured, and naming the wron ones is worse than naming none — with neither available, the line is omitted rather than guessed. +## Two labels, because they expire at different times + +`breaking-change` is a **fact about the commits**. Approving does not make the +break go away, and somebody filtering for that label three months from now wants +this pull request in the results. It is applied in every state this composite +handles, and only the caller removes it — at the one moment the fact stops being +true, when the breaking commits are gone. + +`awaiting-breaking-approval` is a **state**. It goes on while a decision is +pending and comes off the moment one is made, either way. + +Collapsing the two is tempting and wrong: removing `breaking-change` on approval +erases the record exactly when it starts to matter, because now the break is +both real and agreed. + ## What it does not do It never closes, merges or approves anything. `close-on-rejection` only changes @@ -113,7 +128,8 @@ side effect in the caller is what makes this safe to run with | `rejecters` | Comma-separated logins that rejected. | No | `''` | | `rejection-comment` | Comment left with the rejection. | No | `''` | | `close-on-rejection` | Whether the caller closes on rejection. Wording only. | No | `true` | -| `label` | Applied while waiting, removed once resolved. Cosmetic. | No | `''` | +| `label` | States that the PR carries a breaking change. A fact — stays once applied. Cosmetic. | No | `''` | +| `pending-label` | States that the breaking change is waiting on a decision. Removed once approved or rejected. | No | `''` | | `enabled` | `false` skips commenting while still managing the label. | No | `true` | ## Outputs diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 977bddf3..b21f6e18 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -80,7 +80,11 @@ inputs: required: false default: 'true' label: - description: Label applied while waiting and removed once resolved. Cosmetic — failing to apply it never fails the step. Empty disables it. + description: 'Label stating that this pull request carries a breaking change. A fact about the change, so it stays once applied — approving does not make the break go away. The caller removes it when the breaking commits are gone. Cosmetic; failing to apply it never fails the step. Empty disables it. Behaviour change: it used to be removed on a decision — a caller using it as a temporary waiting marker should move that value to `pending-label` and clear this one.' + required: false + default: '' + pending-label: + description: 'Label stating that the breaking change is waiting on a decision. A state, so it is removed once approved or rejected. Kept separate from `label` because "this breaks things" and "nobody has decided yet" stop being true at different moments. Empty disables it.' required: false default: '' enabled: @@ -120,6 +124,7 @@ runs: REJECTION_COMMENT: ${{ inputs.rejection-comment }} CLOSE_ON_REJECTION: ${{ inputs.close-on-rejection }} LABEL: ${{ inputs.label }} + PENDING_LABEL: ${{ inputs.pending-label }} ENABLED: ${{ inputs.enabled }} run: | set -euo pipefail @@ -356,14 +361,30 @@ runs: fi # Cosmetic and deliberately last — a repository that never declared the - # label still gets its gate. + # labels still gets its gate. + # + # Two of them, because they stop being true at different moments. The + # breaking change is a property of the commits: approving it does not + # make the break go away, and somebody filtering for `breaking-change` + # three months from now wants this pull request in the results. Waiting + # on a decision is a state, and it ends. + # + # Every state reaching this composite means a breaking change is + # present — the caller skips it entirely when there is none — so the + # fact label is applied throughout and only the caller removes it. if [ -n "$LABEL" ]; then + if ! gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "$LABEL" >/dev/null 2>&1; then + echo "::notice::Label '${LABEL}' not applied — it is not declared in ${REPO}. Cosmetic only." + fi + fi + + if [ -n "$PENDING_LABEL" ]; then if [ "$STATE" = "waiting" ]; then - if ! gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "$LABEL" >/dev/null 2>&1; then - echo "::notice::Label '${LABEL}' not applied — it is not declared in ${REPO}. Cosmetic only." + if ! gh pr edit "$PR_NUMBER" --repo "$REPO" --add-label "$PENDING_LABEL" >/dev/null 2>&1; then + echo "::notice::Label '${PENDING_LABEL}' not applied — it is not declared in ${REPO}. Cosmetic only." fi else - gh pr edit "$PR_NUMBER" --repo "$REPO" --remove-label "$LABEL" >/dev/null 2>&1 || true + gh pr edit "$PR_NUMBER" --repo "$REPO" --remove-label "$PENDING_LABEL" >/dev/null 2>&1 || true fi fi diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index d731c86c..23c8255f 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -347,6 +347,79 @@ def run_verdict(**overrides): return result, parse_github_output(output_path.read_text(encoding="utf-8")) +def run_reporter_labels(state, **overrides): + """Run the reporter end to end and return the `gh pr edit` calls it made. + + `render_reporter` stops before the label block, so asserting on the + composite's text was the only coverage the lifecycle had — and text cannot + tell which state made which call. Moving `--add-label "$LABEL"` into the + `waiting` branch would keep every string in place and break the behaviour. + """ + action = yaml.safe_load( + ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + ) + body = action["runs"]["steps"][0]["run"] + + env = { + "REPO": "LerianStudio/example", + "PR_NUMBER": "42", + "STATE": state, + "REASON": "rule develop <- '*'", + "HEAD_SHA": "abc1234567", + "ACKNOWLEDGED": "false", + "ACK_LINE": "Breaking change acknowledged: …", + "RUN_URL": "https://github.com/LerianStudio/example/actions/runs/1", + "REVIEWERS": "team:devops", + "SERVER_URL": "https://github.com", + "BADGE_URL": "https://img.shields.io/badge/x-1f6feb", + "DOCS_URL": "https://example.invalid/docs", + "APPROVERS": "carol", + "REJECTERS": "dave", + "REJECTION_COMMENT": "", + "CLOSE_ON_REJECTION": "true", + "LABEL": "breaking-change", + "PENDING_LABEL": "awaiting-breaking-approval", + "ENABLED": "false", + "FINGERPRINT": "", + "RUN_ID": "1", + "GH_TOKEN": "stub", + } + env.update(overrides) + + with tempfile.TemporaryDirectory() as temp_dir: + calls = Path(temp_dir) / "calls" + calls.touch() + stub = Path(temp_dir) / "gh" + stub.write_text( + "#!/usr/bin/env bash\n" + 'case "$*" in\n' + " *\"pr edit\"*)" + " printf '%s\\n' \"$*\" | grep -oE '(add|remove)-label [a-z-]+'" + f' >> "{calls}" ;;\n' + " *\"/commits\"*) printf '%s\\n' 'feat(x)!: breaks' ;;\n" + " *) exit 1 ;;\n" + "esac\n", + encoding="utf-8", + ) + stub.chmod(0o755) + env["PATH"] = f"{temp_dir}:{os.environ.get('PATH', '/usr/bin:/bin')}" + env["GITHUB_OUTPUT"] = str(Path(temp_dir) / "out") + result = subprocess.run( + ["bash", "-c", body], + cwd=temp_dir, + env=env, + text=True, + capture_output=True, + check=False, + ) + if result.returncode != 0: + raise AssertionError(f"reporter failed for {state!r}: {result.stderr}") + return calls.read_text(encoding="utf-8").split() + + def parse_github_output(output): values = {} for line in output.splitlines(): @@ -1219,6 +1292,51 @@ def test_an_unevaluated_acknowledgement_is_not_reported_as_missing(self): self.assertIn("Author acknowledgement", rendered) self.assertIn("missing", rendered) + def test_the_fact_label_outlives_the_decision(self): + # Run it, per state, and read the `gh pr edit` calls. Asserting on the + # composite's text cannot tell which state made which call: moving + # `--add-label "$LABEL"` into the `waiting` branch would leave every + # string in place and stop the fact label from surviving a decision. + for state in ("waiting", "approved", "rejected", "acknowledgement"): + with self.subTest(state=state): + calls = run_reporter_labels(state) + pairs = list(zip(calls[::2], calls[1::2])) + + self.assertIn( + ("add-label", "breaking-change"), + pairs, + "the fact label belongs on every state the gate reports", + ) + self.assertNotIn( + ("remove-label", "breaking-change"), + pairs, + "approving does not make the break go away", + ) + + expected = "add-label" if state == "waiting" else "remove-label" + self.assertIn((expected, "awaiting-breaking-approval"), pairs) + + def test_the_two_labels_can_be_turned_off_independently(self): + # Cosmetic throughout: a repository that declared neither still gets + # its gate, and one that wants only the fact can say so. + self.assertEqual([], run_reporter_labels("waiting", LABEL="", PENDING_LABEL="")) + self.assertEqual( + ["add-label", "breaking-change"], + run_reporter_labels("waiting", PENDING_LABEL=""), + ) + + def test_both_labels_go_when_the_break_does(self): + body = extract_run_body( + extract_job(APPROVAL_WORKFLOW, "outcome"), "Resolve the verdict" + ) + # The one moment the fact stops being true. This cleanup was lost in an + # earlier rewrite, leaving `breaking-change` on a pull request whose + # breaking commit had been reworded away. + self.assertIn('for stale in "$LABEL" "$PENDING_LABEL"', body) + none_at = body.index("state=none") + cleanup_at = body.index('for stale in "$LABEL" "$PENDING_LABEL"') + self.assertLess(none_at, cleanup_at) + def test_the_fingerprint_comes_from_the_breaking_commits(self): detect = extract_job(APPROVAL_WORKFLOW, "detect") self.assertIn("src/validate/breaking-change-commits@", detect)