From e6106f6e1ebaba39ad54a11ca0b0baed47f6ffc4 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:10:57 -0300 Subject: [PATCH 1/3] fix(pr-validation): make the breaking-change comment read as information MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The waiting comment opened with a siren and spent four paragraphs explaining itself. A breaking change is often deliberate, and a comment that shouts teaches people to scroll past it — which is the one thing a gate cannot afford. Now: what was found, a link straight to the decision, and who can make it. Everything else — the scope rule, the acknowledgement state, how to undo the break — is one small line at the bottom. Eighteen lines down to nine. Four things are links now where there were none: the offending commit, the pending deployment, the reviewers, and the docs. "Who can approve this" was the obvious next question and the answer was buried in Environment settings nobody thinks to open. Reviewers render as links, never as @mentions: the pending deployment already notifies exactly those people. --- .../workflows/breaking-change-approval.yml | 1 + .../README.md | 27 +++++ .../action.yml | 106 ++++++++++++------ .../breaking-change-guard/test-workflow.py | 18 +++ 4 files changed, 120 insertions(+), 32 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 6112fdea..215c0a51 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -392,6 +392,7 @@ 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 }} 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..ff4d28f7 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -27,6 +27,30 @@ 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`](#) 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](#) +> +> Who can: [@LerianStudio/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. + ## What it does not do It never closes, merges or approves anything. `close-on-rejection` only changes @@ -46,6 +70,9 @@ 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 the `team:` / `user:` list the Environment was given. | 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 | the gate's docs page | | `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..e5f5b79f 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -31,6 +31,18 @@ inputs: description: Link to the run carrying the pending deployment. required: false default: '' + reviewers: + description: 'Who can answer, as the `team:` / `user:` list the Environment was given. Rendered as GitHub mentions so the reader knows who to ask instead of hunting through settings.' + 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 +88,9 @@ runs: HEAD_SHA: ${{ inputs.head-sha }} ACKNOWLEDGED: ${{ inputs.acknowledged }} RUN_URL: ${{ inputs.run-url }} + 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 +111,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..058c3a07 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -821,6 +821,24 @@ 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. + self.assertNotIn('}@${owner}/', reporter) + 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) From 8db3d0256c0b62ce5a4c8fd6a64a206a83d1beda Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 17:26:28 -0300 Subject: [PATCH 2/3] fix(pr-validation): name the Environment's real reviewers in the comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the CodeRabbit review on #907. environment-protection now reads back who the Environment is actually gated by and exposes it, and the reporter prefers that over the configured list. The two differ whenever the Environment already existed or was edited since — and naming the wrong approvers is worse than naming none, which is what the comment does when neither is available. Also from the review: the `reviewers` description still said "rendered as GitHub mentions", written before the switch to links and never updated the README gave `docs-url` as "the gate's docs page" where .coderabbit.yml asks for input tables to match action.yml exactly the example's links all pointed at `(#)` the no-bare-mention test asserted one forbidden spelling, so it passed by accident and would keep passing if the mention were written any other way. It now checks that every `@${owner}/` sits inside Markdown link text — verified by injecting a bare mention and watching it fail --- .../workflows/breaking-change-approval.yml | 2 ++ .../README.md | 21 ++++++++++++------- .../action.yml | 8 +++++-- .../breaking-change-guard/test-workflow.py | 13 +++++++++++- src/validate/environment-protection/README.md | 1 + .../environment-protection/action.yml | 17 +++++++++++++++ 6 files changed, 52 insertions(+), 10 deletions(-) diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 215c0a51..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' @@ -393,6 +394,7 @@ jobs: 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 ff4d28f7..2092f48e 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -35,13 +35,14 @@ links straight to the decision, and names who can make it: > ### Breaking change — needs a decision > -> [`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. +> [`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](#) +> ### ➡️ [Approve or reject](https://github.com/LerianStudio/example/actions/runs/1) > -> Who can: [@LerianStudio/g_github_devops](#) +> 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 @@ -51,6 +52,11 @@ of the way of whoever does not. 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 @@ -70,9 +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 the `team:` / `user:` list the Environment was given. | 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 | the gate's docs page | +| `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 e5f5b79f..42a6bf76 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -32,7 +32,11 @@ inputs: required: false default: '' reviewers: - description: 'Who can answer, as the `team:` / `user:` list the Environment was given. Rendered as GitHub mentions so the reader knows who to ask instead of hunting through settings.' + 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: @@ -88,7 +92,7 @@ runs: HEAD_SHA: ${{ inputs.head-sha }} ACKNOWLEDGED: ${{ inputs.acknowledged }} RUN_URL: ${{ inputs.run-url }} - REVIEWERS: ${{ inputs.reviewers }} + REVIEWERS: ${{ inputs.actual-reviewers != '' && inputs.actual-reviewers || inputs.reviewers }} SERVER_URL: ${{ inputs.server-url }} DOCS_URL: ${{ inputs.docs-url }} APPROVERS: ${{ inputs.approvers }} diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 058c3a07..6239f0fa 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -837,7 +837,18 @@ def test_the_waiting_comment_links_instead_of_mentioning(self): self.assertIn(fragment, reporter) # The pending deployment already notifies the reviewers; a bare @mention # would ping the whole team again for the same thing. - self.assertNotIn('}@${owner}/', reporter) + # + # 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_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. diff --git a/src/validate/environment-protection/README.md b/src/validate/environment-protection/README.md index a34557ab..ac97bcb6 100644 --- a/src/validate/environment-protection/README.md +++ b/src/validate/environment-protection/README.md @@ -90,6 +90,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..8f2fdde2 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 @@ -62,6 +65,19 @@ runs: --jq '[.protection_rules[].type] | join(",")' 2>/dev/null } + # 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. + read_reviewers() { + gh api "repos/${REPO}/environments/${ENVIRONMENT}" \ + --jq '[.protection_rules[] + | select(.type == "required_reviewers") + | .reviewers[]? + | if .type == "Team" then "team:" + .reviewer.slug + else "user:" + .reviewer.login end] + | join(",")' 2>/dev/null || true + } + # Resolves `team:` / `user:` / `` into the # {type,id} pairs the Environments API expects. It takes numeric ids # only, so every entry costs a lookup. @@ -171,6 +187,7 @@ runs: { echo "protected=true" echo "created=$created" + echo "reviewers=$(read_reviewers)" } >> "$GITHUB_OUTPUT" if [ "$created" != "true" ]; then From 84a9db4f439d01e486a365fa0b361d51d65318b7 Mon Sep 17 00:00:00 2001 From: Lucas Bedatty <79675696+bedatty@users.noreply.github.com> Date: Thu, 15 Oct 2026 18:31:55 -0300 Subject: [PATCH 3/3] fix(pr-validation): read the Environment once, so reviewers cannot be unknown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the second CodeRabbit review on #907. The composite read the Environment twice: once for the protection rules, once for the reviewers, with the second call swallowing failure. That allowed a state of "protected, reviewers unknown" — and an empty reviewer list is indistinguishable from "this Environment has none", so the reporter fell back to the configured list and could name people who cannot actually approve. Which is the very thing the previous commit set out to fix. The suggestion was to carry the read failure as a distinct status. Removing the state is better than reporting it: one GET now answers both questions, so either both are known or the step fails. It also drops a call to an API this job already talks to twice. Test pins it to exactly one read. --- .../breaking-change-guard/test-workflow.py | 12 ++++ src/validate/environment-protection/README.md | 9 +++ .../environment-protection/action.yml | 59 +++++++++++-------- 3 files changed, 57 insertions(+), 23 deletions(-) diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 6239f0fa..941ec476 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -850,6 +850,18 @@ def test_the_waiting_comment_links_instead_of_mentioning(self): 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 ac97bcb6..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 diff --git a/src/validate/environment-protection/action.yml b/src/validate/environment-protection/action.yml index 8f2fdde2..cfdbe555 100644 --- a/src/validate/environment-protection/action.yml +++ b/src/validate/environment-protection/action.yml @@ -60,22 +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. - read_reviewers() { - gh api "repos/${REPO}/environments/${ENVIRONMENT}" \ - --jq '[.protection_rules[] - | select(.type == "required_reviewers") - | .reviewers[]? - | if .type == "Team" then "team:" + .reviewer.slug - else "user:" + .reviewer.login end] - | join(",")' 2>/dev/null || true + 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 @@ -111,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 @@ -167,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 @@ -187,7 +200,7 @@ runs: { echo "protected=true" echo "created=$created" - echo "reviewers=$(read_reviewers)" + echo "reviewers=$(reviewers_of)" } >> "$GITHUB_OUTPUT" if [ "$created" != "true" ]; then