diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 6112fdea..d26bb4c3 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -369,6 +369,7 @@ jobs: ref: ${{ needs.detect.outputs.base-ref }} - name: Verify the breaking-approval Environment is protected + id: environment if: >- needs.detect.result == 'success' && needs.detect.outputs.has-breaking-changes == 'true' @@ -392,6 +393,8 @@ jobs: github-token: ${{ secrets.MANAGE_TOKEN || github.token }} pr-number: ${{ needs.detect.outputs.pr-number }} state: waiting + reviewers: ${{ inputs.approval_reviewers != '' && inputs.approval_reviewers || steps.codeowners.outputs.reviewers }} + actual-reviewers: ${{ steps.environment.outputs.reviewers }} reason: ${{ needs.detect.outputs.reason }} head-sha: ${{ needs.detect.outputs.head-sha }} acknowledged: ${{ needs.detect.outputs.acknowledged }} diff --git a/src/notify/breaking-change-approval-reporter/README.md b/src/notify/breaking-change-approval-reporter/README.md index 516ff460..2092f48e 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -27,6 +27,36 @@ comment, and it ends as the record of who decided what — nothing is deleted. Any other value fails the step. A typo in `state` must not produce a confidently worded comment about the wrong thing. +## It reads as information, not as a block + +A breaking change is often deliberate, and a comment that opens with a siren +teaches people to scroll past it. The waiting comment states what was found, +links straight to the decision, and names who can make it: + +> ### Breaking change — needs a decision +> +> [`f8d5506`](https://github.com/LerianStudio/example/commit/f8d5506) carries a +> breaking change, so merging makes the next release a **major** version. That is +> often deliberate — this just asks someone to confirm it. +> +> ### ➡️ [Approve or reject](https://github.com/LerianStudio/example/actions/runs/1) +> +> Who can: +> [@LerianStudio/g_github_devops](https://github.com/orgs/LerianStudio/teams/g_github_devops) + +Everything else — the scope rule, the acknowledgement state, how to undo the +break — is one `` line at the bottom, present for whoever needs it and out +of the way of whoever does not. + +**Reviewers are rendered as links, never as @mentions.** The pending deployment +already notifies exactly these people; a mention would ping the whole team a +second time for the same thing. + +**`actual-reviewers` wins over `reviewers`.** An Environment that already existed +may be gated by different people than the caller configured, and naming the wrong +ones is worse than naming none — with neither available, the line is omitted +rather than guessed. + ## What it does not do It never closes, merges or approves anything. `close-on-rejection` only changes @@ -46,6 +76,10 @@ side effect in the caller is what makes this safe to run with | `head-sha` | Head commit the report refers to. | No | `''` | | `acknowledged` | Whether the author acknowledgement line is present. | No | `false` | | `run-url` | Link to the run carrying the pending deployment. | No | `''` | +| `reviewers` | Who can answer, as a `team:` / `user:` list. | No | `''` | +| `actual-reviewers` | Reviewers read from the Environment itself. Preferred over `reviewers` when present. | No | `''` | +| `server-url` | Base URL used to build commit links. | No | `${{ github.server_url }}` | +| `docs-url` | Link to the explanation of how the gate works. | No | `https://github.com/LerianStudio/github-actions-shared-workflows/blob/main/docs/breaking-change-approval.md` | | `approvers` | Comma-separated logins that approved. | No | `''` | | `rejecters` | Comma-separated logins that rejected. | No | `''` | | `rejection-comment` | Comment left with the rejection. | No | `''` | diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 19b33ac0..42a6bf76 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -31,6 +31,22 @@ inputs: description: Link to the run carrying the pending deployment. required: false default: '' + reviewers: + description: 'Who can answer, as a `team:` / `user:` list. Rendered as links, never as @mentions — the pending deployment already notifies these people, so a mention would ping them twice for the same thing.' + required: false + default: '' + actual-reviewers: + description: 'Reviewers read from the Environment itself, in the same format. Preferred over `reviewers` when present: an Environment that already existed may be gated by different people than the caller configured, and naming the wrong ones is worse than naming none.' + required: false + default: '' + server-url: + description: Base URL used to build commit links. + required: false + default: ${{ github.server_url }} + docs-url: + description: Link to the explanation of how the gate works. + required: false + default: 'https://github.com/LerianStudio/github-actions-shared-workflows/blob/main/docs/breaking-change-approval.md' approvers: description: Comma-separated logins that approved. required: false @@ -76,6 +92,9 @@ runs: HEAD_SHA: ${{ inputs.head-sha }} ACKNOWLEDGED: ${{ inputs.acknowledged }} RUN_URL: ${{ inputs.run-url }} + REVIEWERS: ${{ inputs.actual-reviewers != '' && inputs.actual-reviewers || inputs.reviewers }} + SERVER_URL: ${{ inputs.server-url }} + DOCS_URL: ${{ inputs.docs-url }} APPROVERS: ${{ inputs.approvers }} REJECTERS: ${{ inputs.rejecters }} REJECTION_COMMENT: ${{ inputs.rejection-comment }} @@ -96,58 +115,85 @@ runs: body_file=$(mktemp) trap 'rm -f "$body_file"' EXIT INT TERM + owner=${REPO%%/*} + + # `team:x,user:y` -> linked names. The reader's next question is always + # "who can unblock this", and the answer is sitting in the Environment + # where nobody thinks to look. + # + # Links, deliberately not @mentions: the pending deployment already + # notifies these very people, and a mention would ping the whole team a + # second time for the same thing. + mentions="" + while IFS= read -r entry; do + [ -z "$entry" ] && continue + case "$entry" in + team:*) + slug=${entry#team:} + mentions="${mentions:+$mentions, }[@${owner}/${slug}](${SERVER_URL}/orgs/${owner}/teams/${slug})" + ;; + *) + login=${entry#user:} + mentions="${mentions:+$mentions, }[@${login}](${SERVER_URL}/${login})" + ;; + esac + done < <(tr ',' '\n' <<< "$REVIEWERS" | xargs -n1 2>/dev/null) + + commit_link="" + if [ -n "$HEAD_SHA" ]; then + commit_link="[\`${HEAD_SHA:0:7}\`](${SERVER_URL}/${REPO}/commit/${HEAD_SHA})" + fi + # shellcheck disable=SC2016 # the backticks below are Markdown code spans, not command substitutions { printf '\n\n' case "$STATE" in waiting) - printf '### 🚨 Breaking change — approval required\n\n' - printf 'A breaking-change commit was found on this branch, so merging it will force the next release to a **major** version.\n\n' - printf 'This pull request is held until a reviewer answers one question: **is this breaking change necessary?**\n\n' - [ -n "$RUN_URL" ] && printf '👉 **[Review the pending deployment](%s)** — *Review deployments* → `breaking-approval`.\n\n' "$RUN_URL" + printf '### Breaking change — needs a decision\n\n' + printf '%s carries a breaking change, so merging makes the next release a **major** version. That is often deliberate — this just asks someone to confirm it.\n\n' \ + "${commit_link:-This branch}" + [ -n "$RUN_URL" ] && printf '### ➡️ [Approve or reject](%s)\n\n' "$RUN_URL" + [ -n "$mentions" ] && printf 'Who can: %s\n\n' "$mentions" ;; approved) printf '### ✅ Breaking change approved\n\n' - printf 'Approved by **%s** — the break is considered necessary and the next release will be a major version.\n\n' \ + printf 'Approved by **%s**. The next release will be a major version.\n\n' \ "${APPROVERS:-a reviewer}" ;; rejected) - printf '### ⛔ Breaking change rejected\n\n' - printf 'Rejected by **%s** — the break is not considered necessary.\n\n' "${REJECTERS:-a reviewer}" - [ -n "$REJECTION_COMMENT" ] && printf '> %s\n\n' "$REJECTION_COMMENT" + printf '### Breaking change rejected\n\n' + printf 'Rejected by **%s**' "${REJECTERS:-a reviewer}" + [ -n "$REJECTION_COMMENT" ] && printf ' — *%s*' "$REJECTION_COMMENT" + printf '.\n\n' + if [ "$CLOSE_ON_REJECTION" = "true" ]; then + printf 'This pull request was closed. The branch is untouched — rework it without the break and reopen.\n\n' + fi ;; unresolved) - printf '### 🚨 Breaking change — approval required\n\n' + printf '### Breaking change — decision not recorded\n\n' printf 'The approval did not complete.' - [ -n "$RUN_URL" ] && printf ' [Open the run](%s) and answer the pending deployment.' "$RUN_URL" + [ -n "$RUN_URL" ] && printf ' [Open the run](%s) to answer it.' "$RUN_URL" printf '\n\n' ;; esac - printf '| | |\n|---|---|\n' - [ -n "$REASON" ] && printf '| Gated via | `%s` |\n' "$REASON" - [ -n "$HEAD_SHA" ] && printf '| Head commit | `%s` |\n' "${HEAD_SHA:0:7}" - printf '| Author acknowledgement in the description | %s |\n' \ - "$([ "$ACKNOWLEDGED" = "true" ] && echo "present" || echo "missing")" - printf '\n' + # One compact line of context instead of a table of three rows. + context="" + [ -n "$REASON" ] && context="Gated by \`${REASON}\`" + if [ "$STATE" = "waiting" ]; then + if [ "$ACKNOWLEDGED" = "true" ]; then + context="${context:+$context · }author acknowledged" + else + context="${context:+$context · }author acknowledgement missing" + fi + fi + [ -n "$context" ] && printf '%s\n\n' "$context" - case "$STATE" in - waiting) - printf '**Approving** records that the approvers consider the break necessary, and releases the merge.\n\n' - if [ "$CLOSE_ON_REJECTION" = "true" ]; then - printf '**Rejecting** records that it is not, and **closes this pull request** so the change can be reworked without the break. Reopening is one click and the branch is untouched.\n\n' - else - printf '**Rejecting** records that it is not, and fails this check.\n\n' - fi - printf 'If the break was unintentional, reword the commits — drop the `!` after the type and remove any `BREAKING CHANGE:` footer — and push again. No approval is needed then.\n' - ;; - rejected) - if [ "$CLOSE_ON_REJECTION" = "true" ]; then - printf 'This pull request was closed. Rework the change so it does not break the contract — drop the `!` and any `BREAKING CHANGE:` footer once it no longer applies — then reopen it. The branch is untouched.\n' - fi - ;; - esac + if [ "$STATE" = "waiting" ]; then + printf 'Not intended? Drop the `!` and any `BREAKING CHANGE:` footer from the commits and push again — the gate clears itself. ' + printf '[How this works](%s)\n' "$DOCS_URL" + fi } > "$body_file" comment_id="" diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 3a1cdb77..941ec476 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -821,6 +821,47 @@ def test_reviewers_come_from_codeowners_on_the_base_ref(self): preflight, ) + def test_the_waiting_comment_links_instead_of_mentioning(self): + reporter = ( + REPO_ROOT / "src" / "notify" / "breaking-change-approval-reporter" + / "action.yml" + ).read_text(encoding="utf-8") + # Links to the commit, the decision, the reviewers and the docs. + for fragment in ( + "${SERVER_URL}/${REPO}/commit/${HEAD_SHA}", + "[Approve or reject](%s)", + "${SERVER_URL}/orgs/${owner}/teams/${slug}", + "[How this works](%s)", + ): + with self.subTest(fragment=fragment): + self.assertIn(fragment, reporter) + # The pending deployment already notifies the reviewers; a bare @mention + # would ping the whole team again for the same thing. + # + # Checked as "every occurrence sits inside Markdown link text", not as + # one forbidden spelling: an assertion pinned to the exact characters + # around `@` passes the day somebody writes the mention differently, + # which is worse than no assertion at all. + for match in re.finditer(r"@\$\{owner\}/", reporter): + preceding = reporter[: match.start()] + self.assertTrue( + preceding.endswith("["), + f"bare team mention at offset {match.start()}: " + f"...{reporter[max(0, match.start() - 40):match.end() + 20]}...", + ) + + def test_the_environment_is_read_exactly_once(self): + # Two reads would allow "rules fetched, reviewers not" — and an empty + # reviewer list is indistinguishable from "this Environment has none", + # so a caller would fall back to whoever it had configured and name the + # wrong people. One read, both answers, no such state. + reads = ENVIRONMENT_PROTECTION.count( + 'gh api "repos/${REPO}/environments/${ENVIRONMENT}"' + ) + self.assertEqual(1, reads, "the Environment must be read exactly once") + self.assertIn("reviewers_of()", ENVIRONMENT_PROTECTION) + self.assertIn("rules_of()", ENVIRONMENT_PROTECTION) + def test_a_missing_environment_is_told_apart_from_an_unreadable_one(self): # Creating on top of a 403 would be guessing; only a 404 is ours to fix. self.assertIn('*"HTTP 404"* | *"Not Found"* )', ENVIRONMENT_PROTECTION) diff --git a/src/validate/environment-protection/README.md b/src/validate/environment-protection/README.md index a34557ab..b2a0a1af 100644 --- a/src/validate/environment-protection/README.md +++ b/src/validate/environment-protection/README.md @@ -46,6 +46,15 @@ drop a wait timer or a deployment branch policy somebody set on purpose, so an existing Environment is reported and left untouched. Creating from nothing is a different act from overwriting somebody's configuration. +### One read, both answers + +The rules and the reviewers come from a single `GET`. Two calls would allow a +state of "protected, reviewers unknown" — and an empty reviewer list is +indistinguishable from *this Environment has none*, so a caller naming the +approvers to a human would quietly fall back to whoever it had configured and +name the wrong people. With one read there is no such state: either both +answers are known, or the step fails. + ### A 404 and a 403 are not the same Only "does not exist" is this composite's to fix. If the Environment merely @@ -90,6 +99,7 @@ Environments API takes numeric ids only. |--------|-------------| | `protected` | `true` when the rule is present. The step fails before it could report `false`. | | `created` | `true` when this run created the Environment. | +| `reviewers` | Who the Environment is actually gated by, as `team:`/`user:`. Read back from the Environment, so it reflects one that already existed with different reviewers than were asked for. | ## Usage diff --git a/src/validate/environment-protection/action.yml b/src/validate/environment-protection/action.yml index 47dd32c8..cfdbe555 100644 --- a/src/validate/environment-protection/action.yml +++ b/src/validate/environment-protection/action.yml @@ -36,6 +36,9 @@ outputs: created: description: "Whether this run created the Environment (true/false)." value: ${{ steps.ensure.outputs.created }} + reviewers: + description: "Reviewers the Environment actually carries, as `team:`/`user:`. Read back from the Environment, so it reflects an existing one that is gated by different people than the caller asked for." + value: ${{ steps.ensure.outputs.reviewers }} runs: using: composite @@ -57,9 +60,34 @@ runs: created=false owner=${REPO%%/*} - read_rules() { - gh api "repos/${REPO}/environments/${ENVIRONMENT}" \ - --jq '[.protection_rules[].type] | join(",")' 2>/dev/null + error_file=$(mktemp) + trap 'rm -f "$error_file"' EXIT INT TERM + environment_json='' + + # One read, both answers. Fetching the rules and the reviewers in + # separate calls would create a state where the first succeeded and the + # second did not — "protected, reviewers unknown" — and an empty + # reviewer list is indistinguishable from "this Environment has none". + # A caller would then fall back to whoever it had configured and name + # the wrong people. There is no such state with a single read. + read_environment() { + environment_json=$(gh api "repos/${REPO}/environments/${ENVIRONMENT}" 2>"$error_file") + } + + rules_of() { + jq -r '[.protection_rules[].type] | join(",")' <<< "$environment_json" + } + + # Who the Environment is actually gated by, which is not necessarily who + # the caller asked for: it may have existed already, or been edited since. + # Callers that name the reviewers to a human should prefer this. + reviewers_of() { + jq -r '[.protection_rules[] + | select(.type == "required_reviewers") + | .reviewers[]? + | if .type == "Team" then "team:" + .reviewer.slug + else "user:" + .reviewer.login end] + | join(",")' <<< "$environment_json" } # Resolves `team:` / `user:` / `` into the @@ -95,19 +123,18 @@ runs: printf '%s' "$resolved" } - if ! rules=$(read_rules); then + if ! read_environment; then # Distinguish "does not exist" from "cannot read". Only the first is # ours to fix; the second is a token problem, and creating on top of # it would be guessing. - if ! error=$(gh api "repos/${REPO}/environments/${ENVIRONMENT}" 2>&1 >/dev/null); then - case "$error" in - *"HTTP 404"* | *"Not Found"* ) ;; - * ) - echo "Could not read the '${ENVIRONMENT}' Environment in ${REPO}: ${error}" >&2 - exit 1 - ;; - esac - fi + error=$(cat "$error_file") + case "$error" in + *"HTTP 404"* | *"Not Found"* ) ;; + * ) + echo "Could not read the '${ENVIRONMENT}' Environment in ${REPO}: ${error}" >&2 + exit 1 + ;; + esac if [ "$CREATE_IF_MISSING" != "true" ]; then echo "The '${ENVIRONMENT}' Environment does not exist in ${REPO}, and create-if-missing is off." >&2 @@ -151,12 +178,14 @@ runs: created=true echo "::notice::Created the '${ENVIRONMENT}' Environment in ${REPO} with reviewers: ${REVIEWERS}" - if ! rules=$(read_rules); then + if ! read_environment; then echo "Created the '${ENVIRONMENT}' Environment but could not read it back." >&2 exit 1 fi fi + rules=$(rules_of) + # Reached whether the Environment was just created or already existed. # An existing one that is unprotected is NOT repaired here: a PUT # replaces the whole configuration, so it would silently drop a wait @@ -171,6 +200,7 @@ runs: { echo "protected=true" echo "created=$created" + echo "reviewers=$(reviewers_of)" } >> "$GITHUB_OUTPUT" if [ "$created" != "true" ]; then