ci: fixes and improvements from CI/CD pipeline review#1603
Open
pragmaxim wants to merge 10 commits into
Open
Conversation
deploy is a fail-fast:false matrix, but wait-for-sync and e2e-tests were gated on needs.deploy.result == 'success'. A single failed coin therefore skipped sync verification and e2e coverage for every coin that DID deploy, leaving freshly deployed instances unverified. Run both jobs on partial failure too; only mode=build (skipped) and cancellation keep them off. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Seven *_test.py files (~1000 lines covering runner.py, build/deploy plans, backend policy and wait_for_sync) existed but were executed by no workflow or Makefile target, so regressions in the pipeline scripts shipped with green CI. Run them via unittest discover in the lint job; they are stdlib-only, need no secrets, and are safe for fork PRs on the GitHub-hosted runner. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Lower SYNC_TIMEOUT_SECONDS to 300 and the job timeout-minutes to 6, keeping the one-minute buffer over the script deadline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Only wait-for-sync had a timeout; every other job fell back to GitHub's 360-minute default, so a hung apt lock, unresponsive backend RPC or stalled install could occupy a self-hosted runner for six hours and block everything queued behind it. Ceilings are sized well above normal runtimes so they only trip on genuine hangs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pass matrix.coin to deploy-bb-and-backend.sh via an env var instead of
expanding ${{ }} directly into the run: script, matching how every other
dynamic value in this workflow is handled. Set persist-credentials: false
on all checkouts so the GITHUB_TOKEN is not left in .git/config on the
persistent self-hosted runners when post-step cleanup is skipped
(cancelled job, crashed runner agent) — testing.yml already does this on
the same runner pool. Nothing in the build/deploy path performs
authenticated git operations after checkout.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Without a concurrency group, every push to a PR queued a full unit->connectivity->integration chain on the limited self-hosted pool even when a newer commit had already superseded it. Cancel-in-progress applies only to pull_request events; push runs on master/develop always run to completion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
git ls-remote arguments are tail-matching glob patterns, so 'mas*' passed validation, and 'master' validated even if only 'feature/master' existed -- after which actions/checkout used the literal (nonexistent or different) ref. Check the returned ref name for an exact match and pass '--' so a ref can never be parsed as a git option. Git forbids glob characters in ref names, so no legitimate ref is rejected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
exc.read() in the HTTPError handlers was unprotected: a connection reset while reading an error body (e.g. proxy 502) raised out of the handler and killed a wait that still had minutes of budget, failing the deploy on one flaky poll. Degrade an unreadable error body to empty instead. Also stop truncating the remaining budget to whole seconds, which declared a timeout with up to a second (and one final poll) left. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The RESOLVED_BRANCH_OR_TAG ternary was copy-pasted as job-level env into all six jobs; a one-sided edit would make jobs resolve different refs. Hoist it to workflow-level env (inputs/github contexts are available there) so it exists once. Also drop the prepare_* coins_csv job outputs, which no downstream job consumes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A dispatched run only showed "Prepare Build Plan -> Build (production_builder)", so the run gave no hint which coins it actually built or deployed. Surface it in three places: a run-name with mode/coins/env/ref so the runs list says it, the coin list in the Build job name, and a runner->coins markdown table written by build_plan.py / deploy_plan.py to GITHUB_STEP_SUMMARY so it renders on the run's summary page. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses the issues found during a review of the
.github/CI/CD pipeline (workflows, composite action, and pipeline scripts). Each issue is fixed in its own commit; changes were limited to fixes and guardrails with no intended behavior changes beyond the ones described below.Correctness fixes
43145215fix(deploy): run post-deploy verification on partial matrix failuredeploy is a fail-fast:false matrix, but wait-for-sync and e2e-tests were gated on
needs.deploy.result == 'success'. A single failed coin therefore skipped sync verification and e2e coverage for every coin that DID deploy, leaving freshly deployed instances unverified. Run both jobs on partial failure too; only mode=build (skipped) and cancellation keep them off.de40df24ci: run .github/scripts unit tests in the lint jobSeven
*_test.pyfiles (~1000 lines covering runner.py, build/deploy plans, backend policy and wait_for_sync) existed but were executed by no workflow or Makefile target, so regressions in the pipeline scripts shipped with green CI. Run them via unittest discover in the lint job; they are stdlib-only, need no secrets, and are safe for fork PRs on the GitHub-hosted runner.f717d9e0fix(ci/cd): require exact ref match in branch/tag validationgit ls-remotearguments are tail-matching glob patterns, somas*passed validation, andmastervalidated even if onlyfeature/masterexisted — after which actions/checkout used the literal (nonexistent or different) ref. Check the returned ref name for an exact match and pass--so a ref can never be parsed as a git option. Git forbids glob characters in ref names, so no legitimate ref is rejected. Covered by three new unit tests.0fb3621afix(ci/cd): keep wait_for_sync retrying through flaky error responsesexc.read()in the HTTPError handlers was unprotected: a connection reset while reading an error body (e.g. proxy 502) raised out of the handler and killed a wait that still had minutes of budget, failing the deploy on one flaky poll. Degrade an unreadable error body to empty instead. Also stop truncating the remaining budget to whole seconds, which declared a timeout with up to a second (and one final poll) left.Hardening / guardrails
137c161bfix(deploy): harden shell interpolation and checkout credentialsPass
matrix.cointo deploy-bb-and-backend.sh via an env var instead of expanding${{ }}directly into therun:script, matching how every other dynamic value in this workflow is handled. Setpersist-credentials: falseon all checkouts so the GITHUB_TOKEN is not left in.git/configon the persistent self-hosted runners when post-step cleanup is skipped (cancelled job, crashed runner agent) — testing.yml already does this on the same runner pool. Nothing in the build/deploy path performs authenticated git operations after checkout.3a7d3a1aci: bound all long-running jobs with timeout-minutesOnly wait-for-sync had a timeout; every other job fell back to GitHub's 360-minute default, so a hung apt lock, unresponsive backend RPC or stalled install could occupy a self-hosted runner for six hours and block everything queued behind it. Ceilings are sized well above normal runtimes so they only trip on genuine hangs.
8fd2c78aci: cancel superseded PR test runs with a concurrency groupWithout a concurrency group, every push to a PR queued a full unit→connectivity→integration chain on the limited self-hosted pool even when a newer commit had already superseded it. Cancel-in-progress applies only to pull_request events; push runs on master/develop always run to completion.
Tuning / cleanup
f39c59b1ci: reduce wait-for-sync timeout from 30 to 5 minutesLower
SYNC_TIMEOUT_SECONDSto 300 and the jobtimeout-minutesto 6, keeping the one-minute buffer over the script deadline.0bbc5e4crefactor(deploy): deduplicate ref resolution and drop unused outputsThe
RESOLVED_BRANCH_OR_TAGternary was copy-pasted as job-level env into all six jobs; a one-sided edit would make jobs resolve different refs. Hoist it to workflow-level env (inputs/github contexts are available there) so it exists once. Also drop theprepare_*coins_csvjob outputs, which no downstream job consumes.UX / visibility
806ee57cfeat(ci/cd): show built/deployed coins in the Actions run UIA dispatched run only showed "Prepare Build Plan → Build (production_builder)", so the run gave no hint which coins it actually built or deployed. Surface it in three places: a
run-namewith mode/coins/env/ref so the runs list says it, the coin list in the Build job name, and a runner→coins markdown table written by build_plan.py / deploy_plan.py toGITHUB_STEP_SUMMARYso it renders on the run's summary page.Noted during review but intentionally not changed here
branch_or_tagcheckout rather than the trusted workflow ref (makes the validation step bypassable and old tags un-deployable) — fixing it would break testing modified plan scripts from a feature branch, so it needs a separate decision.build_packages.pymapsfeature/xandfeature-xto the same staging directory — the path encoding must change in lockstep withdeploy-bb-and-backend.sh.🤖 Generated with Claude Code