ci(demo): build and deploy demo-stack images to the demo server over SSH#1515
Conversation
Independent review of
|
Co-authored-by: Norbert Kulus <norbert.kulus@silksh.pl> Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
…, and deploy invocation Co-authored-by: Norbert Kulus <norbert.kulus@silksh.pl> Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
dd4cdee to
f23c417
Compare
piotrswierzy
left a comment
There was a problem hiding this comment.
Tech Lead Review — PR #1515
Verdict: ❌ Request changes (posted as comment). One functional blocker remains: as written, the build-deploy job cannot execute in the standard tag-push flow, so the pipeline never actually deploys. Everything else is solid and well within scope.
What this PR does
Replaces the disabled if: false placeholder deploy job in .github/workflows/cd.yml with a working build-deploy job (environment demo, self-hosted runner) that builds the four demo images (apps/web/Dockerfile with VITE_API_BASE_URL; root Dockerfile targets production/worker/base), docker saves them to tars, rsyncs the tars + the PrestaShop module + PrestaShop/WooCommerce init scripts to the demo server over SSH, and runs the remote deploy.sh. Workflow-only change (+51/-9, 1 file). Closes #1516.
Overall assessment
No architecture/boundary surface — no TS, no CORE↔Integration concerns, no ORM/migration, no FE. The build targets correctly match the Dockerfiles I checked locally (base/production/worker in root Dockerfile; VITE_API_BASE_URL ARG in apps/web/Dockerfile; docker/woocommerce/ exists). Secrets are all referenced via secrets.*/vars.* — no hardcoded credentials. The earlier fix commit already resolved the WooCommerce-source bug, the bash -s stdin no-op, and the export-step naming, all confirmed in the current diff. The single remaining defect is the trigger gate.
BLOCKING
.github/workflows/cd.yml:14— trigger gate never true on a normal tag push.on.push.branchesis commented out (line 5), so onlyv*tags fire the workflow; butif: github.event.base_ref == 'refs/heads/main'isnullfor the standardgit tag vX && git push origin vXflow, so the job silently skips. Drop theifor useif: startsWith(github.ref, 'refs/tags/v'). This is #1516's first (unchecked) acceptance criterion; a linked successfuldemorun is also still required by #1516.
IMPORTANT
- (none — the previously-flagged WooCommerce sync, deploy invocation, and export-step defects are fixed in the current head.)
SUGGESTION
- cd.yml:5 — delete the commented-out
# branches: [main](and the commentedCopy variables on serverstep) and add a one-line "tags only" comment, so the intended trigger surface is unambiguous. - cd.yml:16 —
actions/checkoutjumps@v4 → @v6in a deploy-only PR; confirmv6is intended and aligned with the other workflows. - Least privilege (non-blocking): consider a top-level
permissions: {}block — the job makes no GitHub API calls, so it doesn't need the defaultGITHUB_TOKENscopes. (ssh-keyscanTOFU host-key handling and third-party-action SHA-pinning were consciously deferred as wontfix for the demo box in #1516 — not re-litigating those.)
Positive observations
- Correct use of
environment: demofor environment-scoped secrets/protection. - Because the trigger is
push(notpull_request), fork PRs can't execute code on the self-hosted runner — the main self-hosted-runner exposure is avoided by design. - Build targets, build-arg, and init-script paths all verified against the repo; scope is tight and matches the issue.
- Prior review findings were genuinely addressed rather than hand-waved; commits are GPG-signed with DCO sign-off.
Once the trigger gate is fixed and one demo run is linked, this is an easy approve.
| - uses: actions/checkout@v4 | ||
| - name: Deployment placeholder | ||
| run: echo 'Deployment configuration coming soon' | ||
| - uses: actions/checkout@v6 |
There was a problem hiding this comment.
SUGGESTION — this bumps actions/checkout from @v4 (baseline cd.yml/ci.yml) straight to @v6 in a deploy-only PR. Please confirm v6 is an intended, published major and align it with the version the other workflows use, so CI/CD don't drift on the same action. (Third-party-action SHA-pinning was consciously deferred in #1516 — this is just about not silently jumping two majors here.)
There was a problem hiding this comment.
I can downgrade actions/checkout but I don't think it's worth. New versions works same way as older and improve CI security
The base_ref check never matched since the workflow only triggers on tag pushes, not branch pushes. Replace it with an explicit empty permissions block and clean up leftover commented-out lines. Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
5d16412 to
62bc6c7
Compare
|
The |
AES-256-CBC has no key verification: decrypting with a random wrong key yields accidentally valid PKCS#7 padding ~0.4% of the time, so the old toThrow assertion failed randomly in CI. The wrong-key test now accepts both legal outcomes (KsefSessionCryptoException or garbage != plaintext), and a new deterministic test covers the exception-wrapping path via a truncated ciphertext, which always fails decipher.final(). No production code changes. Closes #1538 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
|
Fix for the flaky test is up: #1539 (test-only change, verified with a 100k-iteration loop - 0 failures). Once it merges, this flake goes away; in the meantime re-running the failed |
Signed-off-by: norbert-kulus-blockydevs <norbert.kulus@blockydevs.com>
…aes-cipher-wrong-key-test test(ksef): de-flake aes-cipher wrong-key spec
piotrswierzy
left a comment
There was a problem hiding this comment.
Tech Lead Re-review — PR #1515
Re-review after update: prior trigger-gate blocker resolved. ✅ Approve — the build-deploy job can now actually run on the intended trigger, and the second changed file is a clean test-only improvement. Scope stays tight and workflow-only (plus one KSeF spec).
Verdict
The one BLOCKING finding from my prior review (build-deploy job could never execute because if: github.event.base_ref == 'refs/heads/main' is null on a v* tag push) is fixed: the job now carries no if: guard at all, so it runs unconditionally on the v* tag push the workflow fires on (cd.yml:9-10 tags, cd.yml:12-16 job header). The pipeline will now deploy.
Confirmed resolved against the current head
- Trigger gate —
ifclause removed; job runs on tag push. ✅ - Remote deploy invocation —
ssh … "bash ~/deploy.sh"(cd.yml:63) runs the remote script directly; the earlierbash -s deploy.shstdin no-op is gone. ✅ - WooCommerce sync source —
Sync woocommerce init scriptsnow rsyncs./docker/woocommerce/(cd.yml:57), not the PrestaShop path. ✅ - Least-privilege suggestion adopted — top-level
permissions: {}on the job (cd.yml:15), since it makes no GitHub API calls. ✅ - Commented-out
# branches: [main]and the staleCopy variables on serverstep were dropped; trigger surface is now unambiguous (tags only).
Second changed file — no new issue
libs/integrations/ksef/…/aes-cipher.spec.ts (#1538): de-flakes the CBC wrong-key test by accepting either legal outcome — a KsefSessionCryptoException from PKCS#7 padding failure, or a returned garbage plaintext (≠ original) on the ~0.4% of random keys that yield accidentally-valid padding — and adds a deterministic truncated-ciphertext throw case. Correct CBC semantics, tightens rather than weakens the assertion. Good.
Non-blocking notes (unchanged / carried forward)
- SUGGESTION — cd.yml:16
actions/checkout@v6in a deploy-only PR; confirm v6 is intended and aligned with the other workflows (not a blocker). ssh-keyscanTOFU host-key handling and third-party-action SHA-pinning remain consciously deferred for the demo box per #1516 — not re-litigating.- Merge gate, not a code blocker: #1516's acceptance still asks for one linked successful
demoworkflow run — worth attaching before/at merge, but it doesn't hold up the code approval.
Nice turnaround — the prior findings were genuinely addressed rather than papered over.
Summary
deployjob in.github/workflows/cd.ymlwith a workingbuild-deployjob that continuously deploys the demo stack on push tomain.apps/web/DockerfilewithVITE_API_BASE_URL, api/worker/migrate via the rootDockerfiletargetsproduction/worker/base) on a self-hosted runner, transfers them to the demo server over SSH, syncs the PrestaShop module and PrestaShop/WooCommerce init scripts, and triggers the server-sidedeploy.sh.mainto our own demo server.Related issues
Closes #1516
Test plan
main(or re-run the workflow) and confirm thebuild-deployjob runs in thedemoenvironment on the self-hosted runner.apps/prestashop-module/and the init-script directories are in their destination paths, anddeploy.shrestarted the stack.Quality gate
pnpm lintpasses (zero errors)pnpm type-checkpasses (zero errors)pnpm testpasses (all unit tests green)pnpm test:integrationpasses - needed only if you touchedapps/api/test/integration/**or any plugin'sinfrastructure/adapters/.(Workflow-only change - no TS/lint surface touched.)
Migrations
ADR
DCO sign-off
By submitting this pull request, I confirm that my contributions
are made under the terms of the Apache License 2.0, and I
certify the Developer Certificate of Origin
by signing off my commits.