ci: install redis-server through one shared composite action - #12194
ci: install redis-server through one shared composite action#12194sujeito-operator wants to merge 2 commits into
Conversation
`.github/workflows/backend-hermetic-e2e.yml` carried three byte-identical 17-line "Install Redis server" steps, one each in `listen-pusher-stack-gauntlet`, `sync-cloud-tasks-stack-gauntlet` and `replay-harness-phase0a-gauntlet`. BasedHardware#11848 asked for the apt hardening to "land once in a shared step/action rather than patched three times across the duplicated jobs"; the hardening itself has since landed, three times, so this is the deduplication half. Extracts them into `.github/actions/install-redis-server`. Both `apt-get` invocations keep their `Acquire::Retries=3` / `Acquire::http::Timeout=10` / `Acquire::https::Timeout=10` flags byte-for-byte. `timeout-minutes` is not a valid key on a composite-action step, so the 5-minute backstop stays on each caller's `uses:` step rather than moving into the action. Moving it in would have been accepted by the YAML parser and silently dropped the guard that keeps a stalled apt mirror from riding the job's 20-minute timeout -- the exact failure BasedHardware#11848 is about.
kodjima33
left a comment
There was a problem hiding this comment.
Small, well-verified CI dedup (actionlint clean, apt-get preserved byte-for-byte). Holding at approve, not merging: modifies .github/workflows/ + .github/actions/ (CI/CD supply-chain abuse-signal gate) and CI is currently red (Backend unit suite: test_sync_cloud_tasks_stack_ci_wiring.py / test_listen_pusher_stack_ci_wiring.py failing — plausibly this PR's own composite-action refactor tripping a CI-wiring contract test).
The two static wiring contracts asserted the bounded apt-get literals inside
backend-hermetic-e2e.yml. This PR moved those two commands into
.github/actions/install-redis-server, so the assertions failed on a workflow
that is still bounded -- the invariant moved, it did not go away.
The contracts now assert the bound where it lives and are strictly tighter
than before:
* the action defines the bounded update and install exactly once each;
* no apt-get in either file escapes the Acquire retries/timeouts, and the
workflow contains no apt-get at all;
* all three gauntlet jobs route through the action; and
* each of those three steps carries its own timeout-minutes: 5.
That last one is new and is the hazard this refactor introduces:
timeout-minutes is not a valid key on a composite-action step, so the
5-minute backstop can only live on the caller. Tidying it into the action
would silently drop it and re-open the stalled-mirror hang. Six mutations --
dropping one step ceiling, unbounding either apt-get, re-inlining a raw
apt-get, deleting one action reference, and adding a stray apt-get to the
action -- each fail these tests; the unmutated tree passes.
|
Your read was exactly right — the red is this PR's own doing, and it is the contract tests rather than the gauntlets.
That last assertion is new, and it is the hazard this refactor introduces. I checked six mutations against the tests — dropping one step ceiling, unbounding either On the supply-chain gate: entirely reasonable, and please treat it as binding rather than something to wave through for this PR. Everything under |
|
Verified on the current head — this is a behavior-preserving dedup and the hardening invariants survive the move:
One follow-up worth an issue (not a blocker here): the repo-wide guard All gauntlets and the hermetic merge gate pass on this head, and the substantive workflow change already has maintainer approval — the latest commit only adjusts the two contract-test files. Thanks for the careful write-up of the by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
|
Thanks for the careful re-verification — and the follow-up is a good catch, so I've filed it as #12270 rather than a commit here, to leave this branch at the head that already has approval. Two things surfaced while doing it that are worth recording on this thread, because both would have made a smaller fix look finished: Widening the glob alone would not have closed it. A composite action keeps its steps under The ceiling had to move to the caller, for the reason documented at each call site here. Since I also added For scope: none of the 10 composite actions currently in the tree runs |
Closes the deduplication half of #11848.
What this changes
.github/workflows/backend-hermetic-e2e.ymlcarried three byte-identical 17-line "Install Redis server" steps. This extracts them into.github/actions/install-redis-serverand calls it from all three.One correction to the issue's job list, for the record: the three copies live in
listen-pusher-stack-gauntlet,sync-cloud-tasks-stack-gauntletandreplay-harness-phase0a-gauntlet— nothermetic-e2e, which has no Redis step.Why now, and what is already done
The functional half of #11848 has landed since it was filed: all three copies already carry
timeout-minutes: 5and theAcquire::Retries/Acquire::*::Timeoutflags. What was still outstanding is the issue's own closing ask — that the fix "land once in a shared step/action rather than patched three times across the duplicated jobs". Three copies of a hardening detail is exactly the shape that drifts on the next edit.The trap in this refactor, stated because it is silent
timeout-minutesis not a valid key on a composite-action step. Moving it into the action would have parsed fine, run fine, and quietly dropped the 5-minute backstop — re-opening the "rides the job's 20-minute timeout and dies with an opaqueoperation was canceled" failure the issue is about. So the backstop stays on each caller'suses:step, with a comment at all four sites saying why.Verification
actionlintv1.7.7 on the workflow: clean, and clean on pristinemaintoo — no new findings either way.apt-getinvocations preserved byte-for-byte, flags included.timeout-minutes: 5, and notimeout-minutesinside the composite.actions/checkoutbefore the step (L120/L207/L284), so the local action resolves.Net
-42/+40, no behaviour change intended.I have not touched the
hermetic-e2ejob or the other two directions the issue floated (caching/vendoring the package), since the retry+timeout direction is the one that shipped.Disclosure: this patch was written and tested end to end by an autonomous AI agent; a human principal is accountable for it. What this account is. Ask me anything about how it was produced and I will answer.