fix(service-ci): run it-postgres in parallel with build instead of serially#22
Conversation
…rially it-postgres does its own checkout, ECR login, and npm ci — it consumes nothing from the build job's output — so needs: build was pure sequencing, not a real dependency. On spacecat-api-service both jobs run ~5-7min, so removing the artificial ordering roughly halves PR wall-clock CI time (paying max(build, it-postgres) instead of build + it-postgres). downstream jobs (semantic-release, deploy-stage, branch-deploy) keep needs: [build, it-postgres] and are unaffected by this change.
|
Note on rollout: this repo has no automated semantic-release — versions are cut manually, and Opened adobe/spacecat-api-service#2865 as a draft companion PR bumping that pin — it's blocked on this PR merging and a new |
There was a problem hiding this comment.
Hey @aniham,
⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.
Verdict: Approve - clean, well-scoped CI optimization with no blocking concerns.
Complexity: TRIVIAL - 5-line change to a single workflow file.
Changes: Removes needs: build from the it-postgres job so it runs in parallel with build, halving CI wall-clock time (1 file).
Non-blocking (2): minor issues and suggestions
- nit: The
it-postgresinput description (line ~29) still says "after build" but it now runs in parallel - consider updating to "in parallel with build" -.github/workflows/service-ci.yaml:29 - nit: The 4-line comment (lines 194-197) could be a one-liner since timing estimates will drift as jobs evolve -
.github/workflows/service-ci.yaml:195
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 30s | Cost: $2.71 | Commit: 47228870f0f8074c6ad46a73dd77b8869640fc21
If this code review was useful, please react with 👍. Otherwise, react with 👎.
## Summary - Bumps the `service-ci.yaml` pin in `.github/workflows/ci.yaml` from `@v2` to `@v3`. - `@v2` is a frozen floating tag pointing at an old `mysticat-ci` commit; `main` there has since moved to `v3`, which only adds an optional, default-off `mac-giver-caller` input — no breaking changes for consumers (like this repo) that don't set it. ## Why Opened as a companion to [adobe/mysticat-ci#22](adobe/mysticat-ci#22), which removes the artificial `needs: build` dependency on the `it-postgres` job (it doesn't consume anything `build` produces). Measured on this repo's PR runs: `build` ~6m13s, `it-postgres` ~6m36s, run sequentially today for ~13min total. mysticat-ci#22 has merged and shipped as [`v3.1.3`](https://github.com/adobe/mysticat-ci/releases/tag/v3.1.3), with the floating `v3` tag moved to point at it. Re-running this PR's CI confirms the fix: `build` and `it-postgres` now start within 1 second of each other (previously sequential), and the run completed in ~7min total instead of ~13min. ## Test plan - [x] Confirm CI still passes end-to-end on `@v3` with `vpc-enabled: true`, `it-postgres: true`, `bundle-build: true` (no `mac-giver-caller` set, so behavior should be identical to `@v2` until mysticat-ci#22 lands). - [x] After mysticat-ci#22 merges and releases, confirm `build` and `it-postgres` start concurrently on this repo's next PR run and total CI time drops as expected. - [x] Undraft once verified.
Summary
Cuts ~6-7min off PR CI wall-clock time by running
it-postgresin parallel withbuildinstead of after it. Measured onspacecat-api-service:build(lint + unit tests + bundle smoke check) ~6m13s,it-postgres(Docker Postgres integration tests) ~6m36s — sequential today, so every PR pays ~13min. Removing the artificial ordering means payingmax(6m13s, 6m36s)≈ 6.5min instead, roughly halving PR CI time for every service on this reusable workflow withit-postgres: true.it-postgreshadneeds: build, but the job does its own checkout, ECR login, andnpm ci— it doesn't consume anything frombuild's output. The dependency was pure sequencing, not a real one.needs: buildsoit-postgresruns concurrently withbuildinstead of after it.semantic-release,deploy-stage,branch-deploy) keepneeds: [build, it-postgres]and still wait for both — unaffected.Trade-off
needs: buildwasn't purely cosmetic ordering — it also gatedit-postgresbehindbuildsucceeding, so a PR whosebuildjob failed never paid forit-postgres(ECR login + AWS creds + spinning up a Postgres container + ~6-7min of integration tests). That gate wasn't fast, though:npm run lintfails in ~30s, but a failing unit test doesn't surface until the full ~5min parallelmocharun finishes (mocha doesn't bail on first failure by default), sobuilditself typically takes most of its ~6min to report a failure. The gate saved wastedit-postgrescompute on builds that were already going to fail — it didn't make failures fast.After this change,
it-postgresstarts unconditionally in parallel withbuild, so every PR — including ones wherebuildultimately fails — now pays the fullit-postgrescompute cost too, across every consumer of this reusable workflow.This is an intentional trade: faster wall-clock feedback on the (common) passing path, in exchange for higher CI compute/AWS-call cost on the failing path. No change to who can trigger
it-postgresor what credentials it uses — only its timing relative tobuildmoved.Test plan
yaml.safe_load), confirmedit-postgres.needsis now unset and all other jobs unchanged.@v3) thatbuildandit-postgresstart at the same time and total CI time drops as expected.semantic-release/deploy-stage/branch-deploystill correctly wait for bothbuildandit-postgresto succeed before deploying (no regression on fail-fast behavior for deploys).