ci: add aggregate CI OK check so a skipped job cannot pass the gate - #129
ci: add aggregate CI OK check so a skipped job cannot pass the gate#129carole-lavillonniere wants to merge 1 commit into
Conversation
The ruleset requires the `build` check, but `build` has `needs: test`, so a failing `test` leaves `build` reporting "skipped" — and GitHub counts a skipped required check as satisfied. `test` is not itself required, so nothing blocked the merge: six Renovate PRs automerged onto a red `localstack` between Aug 22 and Aug 30. Add a `ci-ok` job depending on every other job that treats any result other than "success" as a failure. Requires a matching ruleset change to require `CI OK` in place of `build`; this commit alone does not close the hole.
|
This PR branches off Under the current ruleset this commit is mergeable: With The red |
|
Closing — this belongs in repository settings, not in a workflow file. Adding An aggregate check earns its place when there are many jobs across several workflows and you want one stable required check name. With two jobs it's over-engineering. The underlying finding stands and is unchanged: |
Problem
Six Renovate PRs (#122–#127) automerged onto a red
localstackbetween Aug 22 and Aug 30. Not one of them had a passing test run.The ruleset
Protect-default-branchrequires two checks:buildhasneeds: test. Whentestfails,buildnever runs and reports conclusionskipped— and GitHub counts a skipped required check as satisfied.testis not itself in the required list, so its failure blocks nothing. The smoke test genuinely passes. Result: the gate goes green over a failing test suite.As it stands
buildis worthless as a gate — it can only ever report success or skipped, never failure-because-tests-failed.testbuildChange
A
ci-okjob (check name CI OK) that depends on every other job in the workflow and fails unless all of them reportsuccess.skippedandcancelledboth fail it.if: always()is essential: without it the aggregator is itself skipped when an upstream job fails, and a required check that never reports leaves the PR pending forever.It reads
needsas JSON rather than enumerating each job, so adding a job toneedsis the only edit required when the workflow grows.The ruleset still requires
build. Until it requires CI OK instead, this job runs and reports but gates nothing. Both halves are needed:CI OKand removebuild. KeepRIE ↔ LocalStack API Smoke Test(a separate workflow, so it can't be aggregated here).Suggested order: land #128 (the actual test fix) first so the branch is green, then this, then the ruleset — otherwise the new gate immediately blocks everything, which is correct behaviour but disruptive if unexpected.
Notes
needs: testonbuildis intentionally kept — no reason to burn build minutes when tests are already failing. The aggregator, not the dependency, is what enforces the gate now.ci-okwill also run in theworkflow_callpath fromweekly-release.yml. It only inspects job results, so the cost is one short runner job.Scan go.mod for known CVEsexists specifically to gate dependency bumps and currently can't block one. Worth a follow-up decision.Verification
YAML parses and the job graph resolves to
test → build → ci-ok. The gating behaviour itself can only be confirmed once this runs on a PR — the run on this PR should show CI OK passing, and #128's branch is the natural place to confirm the failing case if you want it demonstrated before the ruleset flips.