diff --git a/.github/workflows/breaking-change-approval.yml b/.github/workflows/breaking-change-approval.yml index 9183030b..688e27d9 100644 --- a/.github/workflows/breaking-change-approval.yml +++ b/.github/workflows/breaking-change-approval.yml @@ -192,6 +192,22 @@ on: required: false type: boolean default: true + status_context: + description: >- + Name of a commit status published with the gate's verdict. Fixed + across repositories, which a job name cannot be: a reusable + workflow's check surfaces as ` / … / `, and the + caller's job name lives in the consumer's own file — so the same gate + appears under several different names across an organisation and no + single ruleset can require it. + + Set it and the gate publishes that status on every run, so one + required status check covers every repository. Needs MANAGE_TOKEN + carrying `statuses: write`: this job declares its own `permissions:`, + so granting it on the caller has no effect. Empty publishes nothing. + required: false + type: string + default: '' enable_comment: description: Post (and keep updated) a single comment carrying the request, the approval link and the outcome. required: false @@ -724,6 +740,72 @@ jobs: # 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. + # The status goes pending before the wait, and only when there will be + # one. GitHub reads the latest status for a context, so without this the + # previous run's `success` stays current while this run sits at the + # Environment — and a rule requiring it would let the merge through on a + # decision nobody has given yet. + # + # The head SHA does not have to change for that to happen: `edited` + # re-runs on the same commit, and changing a pull request's base moves it + # in or out of scope. A pull request that was out of scope published + # `success`, and retargeting it to `develop` must not keep it. + # + # Only when this run will actually hold. Publishing pending on every run + # would leave the status pending wherever the gate has nothing to ask. + - name: Hold the gate status while the decision is pending + if: >- + inputs.status_context != '' + && needs.detect.result == 'success' + && needs.detect.outputs.in-scope == 'true' + && needs.detect.outputs.has-breaking-changes == 'true' + && steps.reuse.outputs.already-approved != 'true' + && !inputs.dry_run + continue-on-error: true + env: + GH_TOKEN: ${{ secrets.MANAGE_TOKEN || github.token }} + REPO: ${{ github.repository }} + CONTEXT: ${{ inputs.status_context }} + HEAD_SHA: ${{ needs.detect.outputs.head-sha }} + RUN_ID: ${{ github.run_id }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + set -euo pipefail + + [ -n "${HEAD_SHA:-}" ] || exit 0 + + # Who wrote this context last, and from which run? Runs do not finish + # in the order they start: retarget a pull request and the old + # out-of-scope run keeps going, reaches `state=none`, and publishes + # `success` after the new run has already set `pending`. The status + # is the merge decision, so the older run must not have the last + # word. + # + # The run id is in the `target_url` this gate writes, so the ordering + # is readable without asking the runs API. A status written by + # anything else has no id to compare and is left alone. + latest=$( + gh api "repos/${REPO}/commits/${HEAD_SHA}/statuses" \ + --jq "[.[] | select(.context == \"${CONTEXT}\")] | first | .target_url // empty" 2>/dev/null || true + ) + previous_run=$(grep -oE '[0-9]+$' <<< "${latest:-}" || true) + + if [ -n "$previous_run" ] && [ "$previous_run" -gt "$RUN_ID" ] 2>/dev/null; then + echo "::notice::Run ${previous_run} already spoke for '${CONTEXT}' and is newer than this one — leaving it alone" + exit 0 + fi + + if ! gh api --method POST "repos/${REPO}/statuses/${HEAD_SHA}" \ + -f state=pending \ + -f context="$CONTEXT" \ + -f description="Waiting for a decision on the breaking change" \ + -f target_url="$RUN_URL" >/dev/null 2>&1; then + echo "::warning::Could not hold the '${CONTEXT}' status pending. It needs MANAGE_TOKEN carrying \`statuses: write\`." >&2 + exit 0 + fi + + echo "::notice::Status '${CONTEXT}' = pending — waiting for a decision" + - name: Request the approval on the pull request if: >- needs.detect.result == 'success' @@ -814,11 +896,13 @@ jobs: runs-on: ${{ vars.GENERAL_RUNNERS || inputs.runner_type }} needs: [detect, request, approval] if: always() - # Deliberately NOT declaring `actions: read`. A called workflow may only ask - # for permissions the caller already holds, so declaring it would break every - # consumer whose entrypoint does not grant it. The approvals API is read - # through MANAGE_TOKEN when present, and degrades to the job conclusion when - # it is not — see the fallback in the step. + # Deliberately NOT declaring `actions: read`, and for the same reason not + # `statuses: write`. A called workflow may only ask for permissions the + # caller already holds, so declaring either would break every consumer whose + # entrypoint does not grant it. Both are reached through MANAGE_TOKEN + # instead, and both degrade rather than fail — the approvals API falls back + # to the job conclusion, and the commit status warns loudly if it cannot be + # published. permissions: pull-requests: write outputs: @@ -869,6 +953,7 @@ jobs: approved=false result=failure state=unresolved + stage="" rejecters="$REJECTERS" emit() { @@ -876,6 +961,7 @@ jobs: echo "approved=$approved" echo "result=$result" echo "state=$state" + echo "stage=$stage" echo "rejecters=$rejecters" } >> "$GITHUB_OUTPUT" } @@ -971,6 +1057,19 @@ jobs: if [ "$DETECT_RESULT" != "success" ] || [ "$DETECTION_SUCCEEDED" != "true" ] \ || [ "$REQUEST_RESULT" != "success" ]; then state=blocked + + # Which half failed, carried through to the comment. It used to say + # only "the gate did not reach a verdict" and point at the run — + # true, and no help to the author, who then opens the run and hunts + # for the red job. The verdict already knew; it was telling the log + # and nobody else. Measured on a real repository: this is exactly + # what ungoliant-controller's first breaking change produced. + if [ "$DETECT_RESULT" != "success" ] || [ "$DETECTION_SUCCEEDED" != "true" ]; then + stage=detection + else + stage=request + fi + emit echo "::error::Breaking change gate could not run (detect=${DETECT_RESULT}, request=${REQUEST_RESULT}, detection_succeeded=${DETECTION_SUCCEEDED})" exit 0 @@ -1067,6 +1166,10 @@ jobs: github-token: ${{ secrets.MANAGE_TOKEN || github.token }} pr-number: ${{ needs.detect.outputs.pr-number }} state: ${{ steps.verdict.outputs.state }} + # Which half of the gate failed. Without it the comment can only say + # "it did not reach a verdict" and point at the run, leaving the + # author to open it and find the red job. + blocked-stage: ${{ steps.verdict.outputs.stage }} # Only a run whose Environment approval actually happened may write a # record naming itself. A reused approval reports `approved` with the # gate job skipped, and a record pointing at that run would assert @@ -1112,6 +1215,128 @@ jobs: echo "::warning::PR #${PR_NUMBER} could not be closed (token lacks 'pull-requests: write'?) — the gate still failed" fi + # One check name, the same in every repository. A reusable workflow's job + # surfaces as ` / … / Breaking Change Approval`, and the + # caller's job name is in the consumer's own file — measured across the + # organisation, the same gate produces six different names, so no single + # organisation ruleset can require it. A commit status carries no job + # path, which is why CodeRabbit and Socket look identical everywhere. + # + # Published on every run, including the ones with nothing to approve: a + # required status check that never arrives leaves the pull request + # pending forever, which is worse than not having the rule. + - name: Publish the gate status + if: always() && inputs.status_context != '' + continue-on-error: true + env: + GH_TOKEN: ${{ secrets.MANAGE_TOKEN || github.token }} + REPO: ${{ github.repository }} + CONTEXT: ${{ inputs.status_context }} + HEAD_SHA: ${{ needs.detect.outputs.head-sha }} + STATE: ${{ steps.verdict.outputs.state }} + RESULT: ${{ steps.verdict.outputs.result }} + REASON: ${{ needs.detect.outputs.reason }} + APPROVERS: ${{ steps.decisions.outputs.approvers || needs.request.outputs.approved-by }} + REJECTERS: ${{ steps.verdict.outputs.rejecters }} + RUN_ID: ${{ github.run_id }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + DRY_RUN: ${{ inputs.dry_run && 'true' || 'false' }} + run: | + set -euo pipefail + + if [ -z "${HEAD_SHA:-}" ]; then + echo "::warning::No head SHA resolved, so the '${CONTEXT}' status cannot be published. A required check will stay pending." + exit 0 + fi + + # `pending` is never published. The status exists so a branch rule can + # read it, and a rule reading `pending` blocks — this job runs once + # the verdict is in, so it always has a final answer to give. + # Who wrote this context last, and from which run? Runs do not finish + # in the order they start: retarget a pull request and the old + # out-of-scope run keeps going, reaches `state=none`, and publishes + # `success` after the new run has already set `pending`. The status + # is the merge decision, so the older run must not have the last + # word. + # + # The run id is in the `target_url` this gate writes, so the ordering + # is readable without asking the runs API. A status written by + # anything else has no id to compare and is left alone. + latest=$( + gh api "repos/${REPO}/commits/${HEAD_SHA}/statuses" \ + --jq "[.[] | select(.context == \"${CONTEXT}\")] | first | .target_url // empty" 2>/dev/null || true + ) + previous_run=$(grep -oE '[0-9]+$' <<< "${latest:-}" || true) + + if [ -n "$previous_run" ] && [ "$previous_run" -gt "$RUN_ID" ] 2>/dev/null; then + echo "::notice::Run ${previous_run} already spoke for '${CONTEXT}' and is newer than this one — leaving it alone" + exit 0 + fi + + # A superseded run must not touch the pull request, and the status is + # the pull request. Its verdict is about a revision that is no longer + # current, and it carries `result=failure` — so publishing it would + # overwrite the live run's success with a failure and block the pull + # request on a decision that was already made. The same rule the + # comment and the close already follow. + if [ "$STATE" = "superseded" ]; then + echo "::notice::Superseded — leaving the '${CONTEXT}' status to the run that replaced this one" + exit 0 + fi + + case "$STATE" in + approved) + status=success + description="Approved by ${APPROVERS:-a reviewer}" + ;; + rejected) + status=failure + description="Rejected by ${REJECTERS:-a reviewer}" + ;; + none) + status=success + description="No approval needed — ${REASON}" + ;; + blocked) + status=failure + description="The gate could not run — see the run log" + ;; + would-request) + status=success + description="Dry run — would hold for approval" + ;; + *) + status=failure + description="Awaiting a decision on the breaking change" + ;; + esac + + # `result` is the authority on pass or fail; the state only chooses + # the wording. Letting the two disagree is how a status says success + # over a verdict that failed. + [ "$RESULT" = "success" ] || [ "$status" = "failure" ] || status=failure + + if [ "$DRY_RUN" = "true" ]; then + echo "::notice::DRY RUN — would publish status '${CONTEXT}' = ${status} (${description})" + exit 0 + fi + + if ! gh api --method POST "repos/${REPO}/statuses/${HEAD_SHA}" \ + -f state="$status" \ + -f context="$CONTEXT" \ + -f description="${description:0:140}" \ + -f target_url="$RUN_URL" >/dev/null 2>&1; then + # Only MANAGE_TOKEN can fix this. The job declares its own + # `permissions:`, which replaces whatever the caller granted, so + # adding `statuses: write` upstream changes nothing — and the job + # cannot declare it itself without breaking every consumer that + # does not hold it. + echo "::warning::Could not publish the '${CONTEXT}' status. It needs MANAGE_TOKEN carrying \`statuses: write\`; granting it on the caller's job has no effect here. A branch rule requiring this status will stay pending until it can be written." >&2 + exit 0 + fi + + echo "::notice::Status '${CONTEXT}' = ${status} — ${description}" + - name: Record the verdict if: always() env: diff --git a/.github/workflows/go-pr-analysis.yml b/.github/workflows/go-pr-analysis.yml index 6f54339a..b2f2362d 100644 --- a/.github/workflows/go-pr-analysis.yml +++ b/.github/workflows/go-pr-analysis.yml @@ -108,6 +108,17 @@ on: description: 'Enable security scanning (gosec, govulncheck)' type: boolean default: true + fail_on_govulncheck: + description: >- + Fail the Security job when govulncheck reports findings. Default false preserves the + advisory behaviour this step has always had: the Go vulnerability database changes + independently of the pull request, so a newly published CVE in a common dependency would + otherwise turn every repository on this channel red at once, on pull requests that touch + nothing related. Findings are reported either way — as a warning annotation when false, as + an error when true — so the scan is never silent. Turn it on per repository once its + dependency graph is clean. + type: boolean + default: false enable_tests: description: 'Enable unit tests' type: boolean @@ -438,8 +449,22 @@ jobs: - name: Run govulncheck working-directory: ${{ matrix.app.working_dir }} - run: govulncheck ./... - continue-on-error: true + env: + # Data, not script: substituted into the environment, never into the line the shell parses. + FAIL_ON_FINDINGS: ${{ inputs.fail_on_govulncheck }} + APP_NAME: ${{ matrix.app.name }} + # A bare `continue-on-error: true` swallowed the result entirely — the finding stayed in the + # job log, which nobody opens on a green check. Branching on the exit code instead reports it + # as an annotation in both modes, and lets a caller that has cleaned its graph gate on it. + run: | + if govulncheck ./...; then + exit 0 + fi + if [ "$FAIL_ON_FINDINGS" = "true" ]; then + echo "::error::govulncheck reported findings for $APP_NAME" + exit 1 + fi + echo "::warning::govulncheck reported findings for $APP_NAME (advisory: fail_on_govulncheck is false)" # ============================================ # UNIT TESTS diff --git a/.github/workflows/go-pr-validation.yml b/.github/workflows/go-pr-validation.yml index 39bf3da7..8d7ac29d 100644 --- a/.github/workflows/go-pr-validation.yml +++ b/.github/workflows/go-pr-validation.yml @@ -179,6 +179,14 @@ on: description: 'Close the pull request when a reviewer rejects the breaking change.' type: boolean default: true + breaking_change_status_context: + description: >- + Publish the gate's verdict as a commit status under this name, so one + organisation-wide required check can cover every repository. Defaults + to the `BREAKING_APPROVAL_STATUS_CONTEXT` organisation variable. + required: false + type: string + default: '' breaking_change_fallback_reviewers: description: >- Who answers the breaking-change gate in a repository whose CODEOWNERS @@ -248,6 +256,17 @@ on: description: 'Extra arguments passed to golangci-lint (e.g. --timeout=5m)' type: string default: '--timeout=5m' + fail_on_govulncheck: + description: >- + Fail the Security job when govulncheck reports findings. Default false preserves the + advisory behaviour this step has always had: the Go vulnerability database changes + independently of the pull request, so a newly published CVE in a common dependency would + otherwise turn every repository on this channel red at once, on pull requests that touch + nothing related. Findings are reported either way — as a warning annotation when false, as + an error when true — so the scan is never silent. Turn it on per repository once its + dependency graph is clean. + type: boolean + default: false app_name_prefix: description: 'Prefix used to namespace coverage/build artifacts' type: string @@ -482,6 +501,7 @@ jobs: breaking_change_close_on_rejection: ${{ inputs.breaking_change_close_on_rejection }} breaking_change_approval_reviewers: ${{ inputs.breaking_change_approval_reviewers }} breaking_change_fallback_reviewers: ${{ inputs.breaking_change_fallback_reviewers }} + breaking_change_status_context: ${{ inputs.breaking_change_status_context }} breaking_change_create_environment: ${{ inputs.breaking_change_create_environment }} breaking_change_require_protected_environment: ${{ inputs.breaking_change_require_protected_environment }} secrets: @@ -528,6 +548,7 @@ jobs: go_version: ${{ inputs.go_version }} golangci_lint_version: ${{ inputs.golangci_lint_version }} govulncheck_version: ${{ inputs.govulncheck_version }} + fail_on_govulncheck: ${{ inputs.fail_on_govulncheck }} golangci_lint_args: ${{ inputs.golangci_lint_args }} app_name_prefix: ${{ inputs.app_name_prefix }} filter_paths: ${{ inputs.filter_paths }} diff --git a/.github/workflows/js-pr-validation.yml b/.github/workflows/js-pr-validation.yml index 533ccd95..f77a058a 100644 --- a/.github/workflows/js-pr-validation.yml +++ b/.github/workflows/js-pr-validation.yml @@ -187,6 +187,14 @@ on: description: 'Close the pull request when a reviewer rejects the breaking change.' type: boolean default: true + breaking_change_status_context: + description: >- + Publish the gate's verdict as a commit status under this name, so one + organisation-wide required check can cover every repository. Defaults + to the `BREAKING_APPROVAL_STATUS_CONTEXT` organisation variable. + required: false + type: string + default: '' breaking_change_fallback_reviewers: description: >- Who answers the breaking-change gate in a repository whose CODEOWNERS @@ -602,6 +610,7 @@ jobs: breaking_change_close_on_rejection: ${{ inputs.breaking_change_close_on_rejection }} breaking_change_approval_reviewers: ${{ inputs.breaking_change_approval_reviewers }} breaking_change_fallback_reviewers: ${{ inputs.breaking_change_fallback_reviewers }} + breaking_change_status_context: ${{ inputs.breaking_change_status_context }} breaking_change_create_environment: ${{ inputs.breaking_change_create_environment }} breaking_change_require_protected_environment: ${{ inputs.breaking_change_require_protected_environment }} secrets: diff --git a/.github/workflows/pr-validation.yml b/.github/workflows/pr-validation.yml index 0e973b29..a559ab0a 100644 --- a/.github/workflows/pr-validation.yml +++ b/.github/workflows/pr-validation.yml @@ -158,6 +158,19 @@ on: description: 'Close the pull request when a reviewer rejects the breaking change.' type: boolean default: true + breaking_change_status_context: + description: >- + Publish the gate's verdict as a commit status under this name. A + reusable workflow's check carries the caller's job path, so the same + gate appears under a different name in each repository and an + organisation ruleset cannot require it. A commit status does not, + so one name works everywhere. + + Defaults to the `BREAKING_APPROVAL_STATUS_CONTEXT` organisation + variable. Empty publishes nothing. + required: false + type: string + default: '' breaking_change_fallback_reviewers: description: >- Who answers the breaking-change gate in a repository whose CODEOWNERS @@ -342,6 +355,7 @@ jobs: # The caller's value wins; the organisation variable is the default, so # a repository that never heard of this gate still has someone to ask. fallback_reviewers: ${{ inputs.breaking_change_fallback_reviewers != '' && inputs.breaking_change_fallback_reviewers || vars.BREAKING_APPROVAL_FALLBACK_REVIEWERS }} + status_context: ${{ inputs.breaking_change_status_context != '' && inputs.breaking_change_status_context || vars.BREAKING_APPROVAL_STATUS_CONTEXT }} create_approval_environment: ${{ inputs.breaking_change_create_environment }} require_protected_environment: ${{ inputs.breaking_change_require_protected_environment }} dry_run: ${{ inputs.dry_run }} diff --git a/docs/breaking-change-approval.md b/docs/breaking-change-approval.md index 1f2d2cb6..e104d758 100644 --- a/docs/breaking-change-approval.md +++ b/docs/breaking-change-approval.md @@ -498,6 +498,7 @@ Declare them with [`labels-sync`](labels-sync.md), or pass `label: ''` / | `label` | string | `breaking-change` | States that the change is breaking. A fact, so it survives the decision and comes off only when the breaking commits are gone. Cosmetic. | | `pending_label` | string | `awaiting-breaking-approval` | States that a decision is pending. A state, so it comes off once approved or rejected. Cosmetic. | | `enable_approval` | boolean | `true` | Hold breaking changes at the Environment. `false` keeps the gate reporting and labelling but never asks for a decision — the author acknowledgement is a separate control the caller still enforces, and the comment carrying the line to paste comes from here. Not the same as not calling the workflow. | +| `status_context` | string | *(empty)* | Publish the verdict as a commit status under this name — the one check name that is identical in every repository. Through `pr-validation` this defaults to the `BREAKING_APPROVAL_STATUS_CONTEXT` organisation variable. Needs `statuses: write`. Empty publishes nothing. | | `enable_comment` | boolean | `true` | Post and keep the comment updated. | | `dry_run` | boolean | `false` | Report what would be gated without commenting, labelling, requesting an approval, or failing. | | `runner_type` | string | `blacksmith-4vcpu-ubuntu-2404` | Runner label. Overridden by `vars.GENERAL_RUNNERS`. | @@ -575,6 +576,72 @@ if: >- Nothing supplied is still `false` — the gate does not scan out of scope, so there is no answer to report. +## Requiring the gate in a branch rule + +The gate's job surfaces as a check named after the **caller's** job path — +`validate / PR Metadata / Breaking Change Approval / Breaking Change Approval` +in one repository, `validation / Breaking Change Approval / Breaking Change +Approval` in another. The prefix comes from the consumer's own workflow file, +so the same gate appears under several different names across an organisation. +Measured across the repositories consuming the umbrellas: **six distinct +names**. Required status checks match the context exactly, with no wildcard, so +no single organisation ruleset can require it — and a required check that never +arrives leaves every pull request pending forever. + +`status_context` removes the caller from the name. The gate publishes a commit +status, which carries no job path, so the same string works everywhere — the +reason `CodeRabbit` and `Socket Security` look identical in every repository. + +``` +BREAKING_APPROVAL_STATUS_CONTEXT = breaking-change-approval +``` + +Then one organisation ruleset requires `breaking-change-approval` and covers +every repository, whatever its jobs are called. + +Five things worth knowing: + +- **It goes `pending` before the gate waits**, and only then. GitHub reads the + latest status for a context, so without it the previous run's `success` stays + current while this run sits at the Environment — and the rule would allow a + merge on a decision nobody has given. The head SHA does not have to change + for that: `edited` re-runs on the same commit, and retargeting a pull request + moves it in or out of the gate's scope. +- **It is published on every run that reaches a verdict**, including those with + nothing to approve. Out of scope and no breaking commits both publish + `success`. A status that only appears sometimes cannot be required. +- **An older run never speaks over a newer one.** Runs do not finish in the + order they start, and the concurrency group covers the approval job alone. + Each write first reads who wrote the context last — the run id is in the + `target_url` the gate writes — and stops if that run is newer. +- **A superseded run publishes nothing.** Its verdict describes a revision that + is no longer current and carries `result: failure`, so publishing it would + overwrite the live run's success and block the pull request on a decision + already given. The same rule the comment and the close already follow. +- **`result` decides pass or fail**, not the state. The state only chooses the + wording, and the two are not allowed to disagree. +- **It needs `MANAGE_TOKEN` carrying `statuses: write`.** The job declares its + own `permissions:`, which replaces whatever the caller granted — so granting + `statuses: write` upstream has no effect here. Declaring it on the job is not + an option either: a called workflow may only ask for permissions the caller + already holds, so it would break every consumer that does not hold it. Hence + the token. Without one that can write, the step warns and carries on rather + than failing. + + **Confirm the token can write statuses before making this the only required + check.** "It will stay pending" is only true for a commit that has no status + yet. If the same SHA already carries `success` for the context — an earlier + run, before the pull request was retargeted into scope — a failed write + leaves that success current, and a status-only rule allows the merge. The + failure is silent on the pull request by design, since the step must not + fail a job over a check the caller may not require. So either require the + gate's job alongside the status, or check that a run has written the status + at least once before relying on it alone. + +Requiring the job name instead still works for a single repository, and this +repository does exactly that. Prefer the status wherever one rule has to cover +more than one repository. + ## Secrets | Secret | Required | Description | diff --git a/docs/go-pr-analysis.md b/docs/go-pr-analysis.md index 798ca248..19196849 100644 --- a/docs/go-pr-analysis.md +++ b/docs/go-pr-analysis.md @@ -108,6 +108,7 @@ jobs: | `go_version` | Go version to use | No | `1.23` | | `golangci_lint_version` | GolangCI-Lint version | No | `v1.62.2` | | `govulncheck_version` | govulncheck version to install. Pinned rather than `@latest` so an upstream release cannot change what runs here unchosen. Works at any `go_version` — the install step overrides `GOTOOLCHAIN=auto`, the scan still runs under the pinned toolchain | No | `v1.7.0` | +| `fail_on_govulncheck` | Fail the Security job when govulncheck reports findings. Default `false` keeps the scan advisory: the Go vulnerability database changes independently of the pull request, so a newly published CVE in a common dependency would otherwise turn every repository on the channel red at once, on pull requests that touch nothing related. Findings are annotated either way — warning when `false`, error when `true` — so the scan is never silent. Turn it on per repository once its dependency graph is clean | No | `false` | | `golangci_lint_args` | Additional golangci-lint arguments | No | `--timeout=5m` | | `coverage_threshold` | Minimum coverage percentage (0-100) | No | `80` | | `fail_on_coverage_threshold` | Fail if coverage below threshold | No | `true` | @@ -177,6 +178,12 @@ Runs security scanners per changed app: - **GoSec**: Static analysis for security issues (uploads SARIF to GitHub Security tab) - **govulncheck**: Official Go vulnerability database check +GoSec runs in report-only mode (`-no-fail`) and publishes its findings to the Security tab. +govulncheck is advisory by default as well, but always annotates what it finds: a warning when +`fail_on_govulncheck` is `false`, an error — and a failed job — when it is `true`. Set it to `true` +once the repository's dependency graph is clean, so a reachable vulnerability stops being something +only a reader of the job log would notice. + ### tests Runs unit tests per changed app with: - Race detection (`-race`) with `CGO_ENABLED=1` diff --git a/docs/go-pr-validation.md b/docs/go-pr-validation.md index 1bd344d8..94ddafcf 100644 --- a/docs/go-pr-validation.md +++ b/docs/go-pr-validation.md @@ -72,10 +72,12 @@ This distinguishes the two skips that used to look identical: a pipeline the cal | `breaking_change_require_protected_environment` | Refuse to gate when `breaking-approval` has no required reviewers | boolean | `true` | | `breaking_change_approval_reviewers` | Who may answer the gate, as `team:` or `user:`. Empty takes the repository's CODEOWNERS | string | *(empty)* | | `breaking_change_fallback_reviewers` | Who answers when the above is empty **and** CODEOWNERS yields nobody. Defaults to the `BREAKING_APPROVAL_FALLBACK_REVIEWERS` organisation variable | string | *(empty)* | +| `breaking_change_status_context` | Publish the gate verdict as a commit status under this name, so one organisation-wide required check covers every repository. Defaults to the `BREAKING_APPROVAL_STATUS_CONTEXT` organisation variable | string | *(empty)* | | `breaking_change_create_environment` | Create `breaking-approval` when missing. Needs a `MANAGE_TOKEN` with administration rights | boolean | `true` | | `go_version` | Go version | string | `1.23` | | `golangci_lint_version` | GolangCI-Lint version | string | `v1.62.2` | | `govulncheck_version` | govulncheck version to install. Pinned rather than `@latest` so an upstream release cannot change what runs here unchosen. Works at any `go_version` — the install step overrides `GOTOOLCHAIN=auto`, the scan still runs under the pinned toolchain | string | `v1.7.0` | +| `fail_on_govulncheck` | Fail the Security job when govulncheck reports findings. Default `false` keeps the scan advisory: the Go vulnerability database changes independently of the pull request, so a newly published CVE in a common dependency would otherwise turn every repository on the channel red at once, on pull requests that touch nothing related. Findings are annotated either way — warning when `false`, error when `true` — so the scan is never silent. Turn it on per repository once its dependency graph is clean | boolean | `false` | | `golangci_lint_args` | Extra arguments passed to golangci-lint (e.g. `--timeout=5m`) | string | `--timeout=5m` | | `app_name_prefix` | Prefix used to namespace coverage/build artifacts | string | `''` | | `filter_paths` | Newline-separated component path prefixes for monorepo per-component analysis (lint/tests/coverage) **and** security scanning; empty = single-app root run. When using it for security, leave `dockerfile_path` empty so each component Dockerfile is discovered | string | `''` | diff --git a/docs/js-pr-validation.md b/docs/js-pr-validation.md index 7a24e401..41cb820e 100644 --- a/docs/js-pr-validation.md +++ b/docs/js-pr-validation.md @@ -62,6 +62,7 @@ The `Go Analysis` and `Lib Version` stubs are unaffected: they are not flag-guar | `breaking_change_require_protected_environment` | Refuse to gate when `breaking-approval` has no required reviewers | boolean | `true` | | `breaking_change_approval_reviewers` | Who may answer the gate, as `team:` or `user:`. Empty takes the repository's CODEOWNERS | string | *(empty)* | | `breaking_change_fallback_reviewers` | Who answers when the above is empty **and** CODEOWNERS yields nobody. Defaults to the `BREAKING_APPROVAL_FALLBACK_REVIEWERS` organisation variable | string | *(empty)* | +| `breaking_change_status_context` | Publish the gate verdict as a commit status under this name, so one organisation-wide required check covers every repository. Defaults to the `BREAKING_APPROVAL_STATUS_CONTEXT` organisation variable | string | *(empty)* | | `breaking_change_create_environment` | Create `breaking-approval` when missing. Needs a `MANAGE_TOKEN` with administration rights | boolean | `true` | | `node_version` | Node.js version | string | `22` | | `package_manager` | Package manager (`npm`, `yarn`, `pnpm`) | string | `npm` | diff --git a/docs/pr-validation.md b/docs/pr-validation.md index d7fcdf75..e8cff308 100644 --- a/docs/pr-validation.md +++ b/docs/pr-validation.md @@ -183,6 +183,7 @@ jobs: | `breaking_change_require_protected_environment` | boolean | `true` | Refuse to gate when `breaking-approval` has no required reviewers, instead of silently passing | | `breaking_change_approval_reviewers` | string | *(empty)* | Who may answer the gate, as comma-separated `team:` or `user:`. Empty takes the repository's CODEOWNERS | | `breaking_change_fallback_reviewers` | string | *(empty)* | Who answers when the above is empty **and** CODEOWNERS yields nobody — the common case, since most CODEOWNERS have no catch-all `*` rule. Defaults to the `BREAKING_APPROVAL_FALLBACK_REVIEWERS` organisation variable | +| `breaking_change_status_context` | string | *(empty)* | Publish the gate verdict as a commit status under this name, so one organisation-wide required check covers every repository. Defaults to the `BREAKING_APPROVAL_STATUS_CONTEXT` organisation variable | | `breaking_change_create_environment` | boolean | `true` | Create `breaking-approval` when missing. Needs a `MANAGE_TOKEN` with repository administration rights | The breaking change guard has no enable input, target-branch input, acknowledgement input, or opt-out. It applies to every caller and every PR target branch. `dry_run: true` remains a global preview mode without guard enforcement; it is not a guard-specific opt-out and does not change normal `dry_run: false` operation. Existing callers must migrate their `pull_request.types` list to include both `edited` and `ready_for_review`; otherwise body edits and draft-to-ready transitions do not rerun validation. diff --git a/src/docs/helm-upgrade-doc/scripts/generate-doc.sh b/src/docs/helm-upgrade-doc/scripts/generate-doc.sh index 24b37121..43507318 100755 --- a/src/docs/helm-upgrade-doc/scripts/generate-doc.sh +++ b/src/docs/helm-upgrade-doc/scripts/generate-doc.sh @@ -28,13 +28,24 @@ CHART_DIFF=$(git diff "${PREV_TAG}".."${CURRENT_TAG}" -- "${CHART_PATH}/Chart.ya VALUES_DIFF=$(git diff "${PREV_TAG}".."${CURRENT_TAG}" -- "${CHART_PATH}/values.yaml" 2>/dev/null | head -400 || true) TEMPLATE_DIFF=$(git diff "${PREV_TAG}".."${CURRENT_TAG}" -- "${CHART_PATH}/templates/" 2>/dev/null | head -300 || true) +# Liquid guard: charts.lerian.studio is built by GitHub Pages (Jekyll 3.10), which runs +# Liquid over every .md, so Helm template snippets ({{ ... }}) in a guide warn or, when +# malformed, break the whole site build. Generated guides are wrapped in a raw block +# hidden in HTML comments (Liquid honours it, GitHub's markdown view hides it). Markers +# are stripped from examples and model output first: a nested raw breaks Liquid too. +strip_liquid_raw() { + { grep -vE '^[[:space:]]*[[:space:]]*$' || true; } \ + | sed -E 's/\{%-?[[:space:]]*(end)?raw[[:space:]]*-?%\}//g' +} + # Load 2 most recent existing UPGRADE docs as few-shot examples EXAMPLES="" if [ -d "$DOCS_PATH" ]; then # shellcheck disable=SC2012 while IFS= read -r doc_file; do DOC_NAME=$(basename "$doc_file") - DOC_CONTENT=$(cat "$doc_file") + # Drop the Liquid raw markers (see "Liquid guard" above) so the model never copies them. + DOC_CONTENT=$(strip_liquid_raw < "$doc_file") EXAMPLES="$(printf '%s\n=== %s ===\n%s\n' "$EXAMPLES" "$DOC_NAME" "$DOC_CONTENT")" done < <(ls -t "${DOCS_PATH}"/UPGRADE-*.md 2>/dev/null | head -2) fi @@ -168,6 +179,11 @@ if echo "$FIRST_LINE" | grep -q '^```'; then CONTENT=$(echo "$CONTENT" | sed '1d; $d') fi +CONTENT=$(printf '%s\n' "$CONTENT" | strip_liquid_raw) +if printf '%s' "$CONTENT" | grep -qE '\{\{|\{%'; then + CONTENT=$(printf '\n%s\n' "$CONTENT") +fi + mkdir -p "$DOCS_PATH" printf '%s\n' "$CONTENT" > "$DOC_FILE" diff --git a/src/notify/breaking-change-approval-reporter/README.md b/src/notify/breaking-change-approval-reporter/README.md index 45cd2152..f8c36ac5 100644 --- a/src/notify/breaking-change-approval-reporter/README.md +++ b/src/notify/breaking-change-approval-reporter/README.md @@ -24,7 +24,7 @@ comment, and it ends as the record of who decided what — nothing is deleted. | `approved` | Approved by whom, and that the next release will be a major version | | `rejected` | Rejected by whom, the reason they gave, and what to do next | | `unresolved` | The approval ended without a recorded decision — open the run | -| `blocked` | The gate could not reach a verdict at all — detection or the request stage failed, and the check fails closed. Deliberately neutral about which: `blocked` is reached from both. The only state that does **not** apply the fact label, because nothing was established — labels already on the pull request are left alone | +| `blocked` | The gate could not reach a verdict — detection or the request stage failed, and the check fails closed. Pass `blocked-stage` to say which; without it the wording stays neutral, since `blocked` is reached from both. The only state that does **not** apply the fact label, because nothing was established — labels already on the pull request are left alone | Any other value fails the step. A typo in `state` must not produce a confidently worded comment about the wrong thing. @@ -117,6 +117,7 @@ side effect in the caller is what makes this safe to run with | `pr-number` | Pull request to report on. | Yes | — | | `repository` | Repository owning it, as `owner/name`. | No | `${{ github.repository }}` | | `state` | `waiting`, `acknowledgement`, `approved`, `rejected`, `unresolved` or `blocked`. | Yes | — | +| `blocked-stage` | With `state: blocked`, which half failed — `detection` or `request`. Named in the comment so the author knows where to look. | No | `''` | | `reason` | Why the pull request is gated, from `pr-gate-scope`. | No | `''` | | `head-sha` | Head commit the report refers to. | No | `''` | | `acknowledged` | Whether the author acknowledgement line is present. | No | `false` | diff --git a/src/notify/breaking-change-approval-reporter/action.yml b/src/notify/breaking-change-approval-reporter/action.yml index 386bf4d5..736a8249 100644 --- a/src/notify/breaking-change-approval-reporter/action.yml +++ b/src/notify/breaking-change-approval-reporter/action.yml @@ -60,6 +60,14 @@ inputs: resolver first. required: false default: '' + blocked-stage: + description: >- + With `state: blocked`, which half of the gate failed — `detection` or + `request`. Named in the comment so the author knows where to look + instead of opening the run and hunting for the red job. Empty keeps the + generic wording. + required: false + default: '' fingerprint: description: 'Digest of the breaking-change commit set, from `breaking-change-commits`. Recorded in the comment when the state is `approved`, alongside the run that carried the approval, so a later run can verify it against the Environment log rather than trusting the text. Previous records are preserved.' required: false @@ -121,6 +129,7 @@ runs: HEAD_SHA: ${{ inputs.head-sha }} ACKNOWLEDGED: ${{ inputs.acknowledged }} SUBJECTS: ${{ inputs.subjects }} + BLOCKED_STAGE: ${{ inputs.blocked-stage }} ACK_LINE: ${{ inputs.author-acknowledgement }} RUN_URL: ${{ inputs.run-url }} REVIEWERS: ${{ inputs.actual-reviewers != '' && inputs.actual-reviewers || inputs.reviewers }} @@ -360,9 +369,21 @@ runs: printf '> [!CAUTION]\n' printf '> ## 💥 Breaking change gate could not run\n' printf '>\n' - printf '> The gate did not reach a verdict, so this check fails closed — an unknown is not the same as "no breaking changes".' - [ -n "$RUN_URL" ] && printf ' [Open the run](%s) to see which step failed.' "$RUN_URL" - printf '\n\n' + case "$BLOCKED_STAGE" in + detection) + printf '> **Detection did not complete**, so the gate does not know whether this pull request carries a breaking change — and an unknown is not the same as "no breaking changes". This check fails closed.\n' + ;; + request) + printf '> The break was detected, but **the approval could not be requested**, so there is nothing for a reviewer to answer. This check fails closed.\n' + printf '>\n' + printf '> Most often the `breaking-approval` Environment does not exist and could not be created — either no reviewer could be resolved (the repository CODEOWNERS has no catch-all `*` rule and no fallback is configured), or the token cannot create Environments. The job log names which.\n' + ;; + *) + printf '> The gate did not reach a verdict, so this check fails closed — an unknown is not the same as "no breaking changes".\n' + ;; + esac + [ -n "$RUN_URL" ] && printf '>\n> [Open the run](%s) for the failing step.\n' "$RUN_URL" + printf '\n' # `ack_followup` is a no-op unless the acknowledgement was # actually evaluated: the gate sends an empty value, not # `false`, when nothing read the description. diff --git a/src/validate/breaking-change-guard/test-workflow.py b/src/validate/breaking-change-guard/test-workflow.py index 1cb0b6fc..0a76d9af 100644 --- a/src/validate/breaking-change-guard/test-workflow.py +++ b/src/validate/breaking-change-guard/test-workflow.py @@ -75,6 +75,7 @@ "breaking_change_require_protected_environment", "breaking_change_approval_reviewers", "breaking_change_fallback_reviewers", + "breaking_change_status_context", "breaking_change_create_environment", } @@ -261,6 +262,7 @@ def render_reporter(state, **overrides): # composite reads the commits itself and `MOCK_SUBJECTS` feeds the # stubbed API. Pass `SUBJECTS` to exercise the forwarded set instead. "SUBJECTS": "", + "BLOCKED_STAGE": "", "MOCK_SUBJECTS": "", } env.update(overrides) @@ -447,6 +449,69 @@ def run_detect_normalization(**overrides): return outputs +def run_gate_status(**overrides): + """Run the step that publishes the commit status, and report the call. + + Returns (process, calls). The status is the one check name a branch rule + can require across every repository, so what it publishes per state is the + contract — not something to read off the source. + """ + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + step = next( + s + for s in workflow["jobs"]["outcome"]["steps"] + if s.get("name") == "Publish the gate status" + ) + + env = { + "REPO": "LerianStudio/example", + "CONTEXT": "breaking-change-approval", + "HEAD_SHA": "abc1234567", + "STATE": "approved", + "RESULT": "success", + "REASON": "rule develop <- '*'", + "APPROVERS": "carol", + "REJECTERS": "", + "RUN_URL": "https://github.com/LerianStudio/example/actions/runs/1", + "DRY_RUN": "false", + "RUN_ID": "500", + "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" + # The read that decides whether an older run may speak. It answers + # the target_url of whoever wrote the context last; LATEST_RUN sets + # which run that was. + 'case "$*" in\n' + ' *"/statuses"*[!0-9])\n' + ' if [ -n "${LATEST_RUN:-}" ]; then\n' + " printf '%s\\n' \"https://github.com/o/r/actions/runs/${LATEST_RUN}\"\n" + " fi\n" + " exit 0 ;;\n" + "esac\n" + f'printf \'%s\\n\' "$*" >> "{calls}"\n' + 'if [ -n "${STUB_FAILS:-}" ]; then exit 1; fi\n', + encoding="utf-8", + ) + stub.chmod(0o755) + env["PATH"] = f"{temp_dir}:{os.environ.get('PATH', '/usr/bin:/bin')}" + result = subprocess.run( + ["bash", "-c", step["run"]], + cwd=temp_dir, + env=env, + text=True, + capture_output=True, + check=False, + ) + return result, calls.read_text(encoding="utf-8").splitlines() + + def run_reporter_comment( state, existing_body=None, read_fails=False, legacy_ids="", **overrides ): @@ -478,6 +543,7 @@ def run_reporter_comment( "ACKNOWLEDGED": "true", "ACK_LINE": "Breaking change acknowledged: …", "SUBJECTS": "", + "BLOCKED_STAGE": "", "RUN_URL": "https://github.com/LerianStudio/example/actions/runs/1", "REVIEWERS": "team:devops", "SERVER_URL": "https://github.com", @@ -592,6 +658,7 @@ def run_reporter_labels(state, **overrides): "PENDING_LABEL": "awaiting-breaking-approval", "ENABLED": "false", "SUBJECTS": "", + "BLOCKED_STAGE": "", "FINGERPRINT": "", "RUN_ID": "1", "GH_TOKEN": "stub", @@ -2265,6 +2332,223 @@ def test_no_instruction_is_better_than_an_empty_one(self): # The break is still announced — only the unusable instruction is gone. self.assertIn("acknowledgement missing", rendered) + def test_the_gate_publishes_one_status_name_everywhere(self): + # A reusable workflow's check is reported as ` / … / `, + # and the caller's job name lives in the consumer's repository. Measured + # across the organisation, the same gate produces six different names, + # so no single ruleset can require it. A commit status carries no job + # path — which is why CodeRabbit and Socket look identical everywhere. + def published(**overrides): + result, calls = run_gate_status(**overrides) + self.assertEqual(0, result.returncode, result.stderr) + self.assertEqual(1, len(calls), calls) + return calls[0] + + # Pass and fail, by state. Each one carries the `result` the verdict + # actually emits for it — the previous version of this test passed the + # helper's default `success` for every state, which is how it missed + # that `superseded` publishes with `result=failure`. + for state, extra, expected in ( + ("approved", {}, "success"), + ("none", {}, "success"), + ("rejected", {"RESULT": "failure"}, "failure"), + ("blocked", {"RESULT": "failure"}, "failure"), + ("unresolved", {"RESULT": "failure"}, "failure"), + ): + with self.subTest(state): + call = published(STATE=state, **extra) + self.assertIn(f"state={expected}", call.replace("state ", "state=")) + self.assertIn("statuses/abc1234567", call) + self.assertIn("context=breaking-change-approval", call.replace("context ", "context=")) + + # `result` is the authority: a state that reads as success cannot + # publish success over a verdict that failed. + contradictory = published(STATE="none", RESULT="failure") + self.assertIn("failure", contradictory) + + # Published on every run, including the ones with nothing to approve — + # a required check that never arrives leaves the pull request pending + # forever, which is worse than having no rule. + self.assertIn("statuses/", published(STATE="none")) + + def test_an_older_run_cannot_speak_over_a_newer_one(self): + # Runs do not finish in the order they start, and `concurrency` is on + # the approval job only — the rest of an older run keeps going. + # Retarget a pull request to an in-scope base without touching the head + # SHA and the old out-of-scope run reaches `state=none`, then publishes + # `success` after the new run has already set `pending`. GitHub reads + # the latest status for a context, so a status-only rule would allow + # the merge before anyone decided. + # + # `superseded` does not cover this: the head did not move and nothing + # was cancelled, so the old run's verdict is a legitimate `none`. + _, late = run_gate_status(STATE="none", RUN_ID="500", LATEST_RUN="900") + self.assertEqual([], late, "an older run published over a newer one") + + # A newer run still speaks over an older one, which is the whole point + # of the ordering rather than a blanket refusal. + _, newer = run_gate_status(STATE="none", RUN_ID="500", LATEST_RUN="100") + self.assertTrue(newer) + + # And a context nobody has written yet is published normally. + _, first = run_gate_status(STATE="none", RUN_ID="500") + self.assertTrue(first) + + # The hold applies the same rule, or the pending it writes could be + # the thing an older run overwrites. + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + hold = next( + s + for s in workflow["jobs"]["request"]["steps"] + if s.get("name") == "Hold the gate status while the decision is pending" + ) + self.assertIn("previous_run", hold["run"]) + self.assertIn("RUN_ID", hold["env"]) + + def test_a_superseded_run_leaves_the_status_alone(self): + # The verdict emits `superseded` with `result=failure` — confirmed by + # running it — so publishing from such a run would overwrite the live + # run's success with a failure and block the pull request on a decision + # that was already given. GitHub reads the latest status for a context, + # and a superseded run can finish after the one that replaced it. + _, verdict = run_verdict(APPROVAL_RESULT="cancelled", MOCK_NEWER_RUNS="2") + self.assertEqual("superseded", verdict["state"]) + self.assertEqual("failure", verdict["result"]) + + result, calls = run_gate_status( + STATE=verdict["state"], RESULT=verdict["result"] + ) + self.assertEqual(0, result.returncode, result.stderr) + self.assertEqual([], calls, "a superseded run published a status") + + # Same for a head that moved on, the other route to superseded. + _, moved = run_verdict(LIVE_SHA="def456") + _, moved_calls = run_gate_status(STATE=moved["state"], RESULT=moved["result"]) + self.assertEqual([], moved_calls) + + def test_the_status_goes_pending_before_the_gate_waits(self): + # GitHub reads the latest status for a context, so without this the + # previous run's `success` stays current while this run sits at the + # Environment — and a rule requiring it would let the merge through on + # a decision nobody has given. + # + # The head SHA does not have to change for that: `edited` re-runs on + # the same commit, and changing a pull request's base moves it in or + # out of the gate's scope. + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + steps = workflow["jobs"]["request"]["steps"] + names = [s.get("name") for s in steps] + hold = next( + s for s in steps if s.get("name") == "Hold the gate status while the decision is pending" + ) + + # Before the wait, which is the whole point. The hold lives in + # `request`, and `approval` — the job that sits at the Environment — + # needs `request`, so the status is pending before anything waits. + self.assertIn("Hold the gate status while the decision is pending", names) + self.assertIn("request", workflow["jobs"]["approval"]["needs"]) + self.assertLess( + names.index("Hold the gate status while the decision is pending"), + names.index("Request the approval on the pull request") + 1, + ) + + # Only when this run will actually hold: publishing pending wherever + # the gate has nothing to ask would leave the status pending forever. + for condition in ( + "inputs.status_context != ''", + "needs.detect.outputs.in-scope == 'true'", + "needs.detect.outputs.has-breaking-changes == 'true'", + "steps.reuse.outputs.already-approved != 'true'", + "!inputs.dry_run", + ): + self.assertIn(condition, hold["if"], condition) + + # Same context as the final status, or the two describe different + # checks and the rule only ever sees one of them. + publish = next( + s + for s in workflow["jobs"]["outcome"]["steps"] + if s.get("name") == "Publish the gate status" + ) + self.assertEqual( + publish["env"]["CONTEXT"], hold["env"]["CONTEXT"] + ) + self.assertIn("state=pending", " ".join(hold["run"].split())) + + def test_the_status_is_opt_in_and_degrades_instead_of_failing(self): + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + step = next( + s + for s in workflow["jobs"]["outcome"]["steps"] + if s.get("name") == "Publish the gate status" + ) + # Off by default: a repository that never asked for it is untouched. + self.assertIn("inputs.status_context != ''", step["if"]) + self.assertEqual( + "", yaml.safe_load(APPROVAL_WORKFLOW)[True]["workflow_call"]["inputs"][ + "status_context" + ]["default"] + ) + + # `statuses: write` is NOT declared on the job. A called workflow may + # only ask for permissions the caller already holds, so declaring it + # would break every consumer whose entrypoint does not grant it — the + # same reasoning the file already applies to `actions: read`. + self.assertNotIn("statuses", workflow["jobs"]["outcome"]["permissions"]) + + # So a token that cannot write it warns and carries on, rather than + # failing the job over a check the caller may not even require. + result, calls = run_gate_status(STUB_FAILS="1") + self.assertEqual(0, result.returncode) + self.assertIn("statuses: write", result.stdout + result.stderr) + + # And a dry run says what it would publish without publishing it. + preview, dry_calls = run_gate_status(DRY_RUN="true") + self.assertEqual(0, preview.returncode) + self.assertEqual([], dry_calls) + self.assertIn("would publish status", preview.stdout) + + def test_a_blocked_gate_names_the_half_that_failed(self): + # Measured on a real repository: ungoliant-controller's first breaking + # change hit this, and the comment said only "did not reach a verdict" + # with a link to the run. True, and no help — the author has to open + # the run and find the red job. The verdict already knew which half + # failed; it was telling the log and nobody else. + detection = render_reporter("blocked", ACKNOWLEDGED="", BLOCKED_STAGE="detection") + self.assertIn("Detection did not complete", detection) + self.assertNotIn("could not be requested", detection) + + request = render_reporter("blocked", ACKNOWLEDGED="", BLOCKED_STAGE="request") + self.assertIn("the approval could not be requested", request) + self.assertNotIn("Detection did not complete", request) + # And the cause that actually happens, named rather than implied. + self.assertIn("breaking-approval", request) + self.assertIn("catch-all", request) + + # Empty keeps the generic wording — a caller that does not pass it is + # no worse off than before. + generic = render_reporter("blocked", ACKNOWLEDGED="", BLOCKED_STAGE="") + self.assertIn("did not reach a verdict", generic) + + # The verdict resolves the stage, and the outcome forwards it. + _, from_detection = run_verdict(DETECTION_SUCCEEDED="false") + self.assertEqual("detection", from_detection["stage"]) + _, from_request = run_verdict(REQUEST_RESULT="failure") + self.assertEqual("request", from_request["stage"]) + # Not set when nothing is blocked — it describes a failure, not a state. + _, approved = run_verdict(APPROVAL_RESULT="success") + self.assertEqual("", approved["stage"]) + + workflow = yaml.safe_load(APPROVAL_WORKFLOW) + step = next( + s + for s in workflow["jobs"]["outcome"]["steps"] + if s.get("name") == "Report the outcome on the pull request" + ) + self.assertEqual( + "${{ steps.verdict.outputs.stage }}", step["with"]["blocked-stage"] + ) + def test_a_blocked_gate_says_so_on_the_pull_request(self): # Detection failing fails the check. It used to do that silently: a red # check with no comment reads as a flake and gets re-run rather than