Skip to content

ci: add aggregate CI OK check so a skipped job cannot pass the gate - #129

Closed
carole-lavillonniere wants to merge 1 commit into
localstackfrom
ci/require-aggregate-check
Closed

ci: add aggregate CI OK check so a skipped job cannot pass the gate#129
carole-lavillonniere wants to merge 1 commit into
localstackfrom
ci/require-aggregate-check

Conversation

@carole-lavillonniere

Copy link
Copy Markdown

Problem

Six Renovate PRs (#122#127) automerged onto a red localstack between Aug 22 and Aug 30. Not one of them had a passing test run.

The ruleset Protect-default-branch requires two checks:

required_status_checks: ["build", "RIE ↔ LocalStack API Smoke Test"]

build has needs: test. When test fails, build never runs and reports conclusion skipped — and GitHub counts a skipped required check as satisfied. test is 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 build is worthless as a gate — it can only ever report success or skipped, never failure-because-tests-failed.

PR Merged test build Gate
#121 Aug 17 pass
#122 Aug 22 ⏭️ skipped pass
#123#127 Aug 22–30 ⏭️ skipped pass

Change

A ci-ok job (check name CI OK) that depends on every other job in the workflow and fails unless all of them report success. skipped and cancelled both 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 needs as JSON rather than enumerating each job, so adding a job to needs is the only edit required when the workflow grows.

⚠️ This PR alone does not close the hole

The ruleset still requires build. Until it requires CI OK instead, this job runs and reports but gates nothing. Both halves are needed:

  1. Merge this PR.
  2. Edit https://github.com/localstack/lambda-runtime-init/rules/18375028 — under required status checks, add CI OK and remove build. Keep RIE ↔ 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: test on build is 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-ok will also run in the workflow_call path from weekly-release.yml. It only inspects job results, so the cost is one short runner job.
  • Trivy is still not a required check. Out of scope here, but Scan go.mod for known CVEs exists 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.

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.
@carole-lavillonniere

Copy link
Copy Markdown
Author

This PR branches off localstack, which still carries the broken test, so its own CI is an unintentional but exact demonstration of the bug and the fix:

CI OK                            fail      ← new gate catches it
test                             fail
build                            skipping  ← old required check, would have counted as PASS
RIE ↔ LocalStack API Smoke Test  pass
Scan go.mod for known CVEs       pass

Under the current ruleset this commit is mergeable: build reports skipping, the smoke test passes, and those are the only two required checks. That is precisely how #122#127 got in.

With CI OK required instead, it is blocked.

The red test here is the pre-existing #122 breakage, not anything this PR introduces — it goes green once #128 lands and this branch is rebased.

@carole-lavillonniere

Copy link
Copy Markdown
Author

Closing — this belongs in repository settings, not in a workflow file.

Adding test to the ruleset's required status checks fixes the same hole with no code, no extra runner job, and nothing to keep in sync. The ci-ok aggregator here doesn't self-maintain either — new jobs still have to be added to needs: — so it only moves the maintenance into YAML and adds a job.

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: build has needs: test, so a failing test makes build report skipped, which GitHub counts as a passing required check — which is how #122#127 automerged onto a red branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant