Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
10 of 12 tasks
tsdk02
force-pushed
the
migrations-sqlx
branch
2 times, most recently
from
September 7, 2026 11:22
bad6094 to
0f8b1ac
Compare
Migrations could not run in a cluster. The SQL never entered the image — the Dockerfile copies only the compiled binary — the runner was hardcoded `psql -f` lines in the justfile needing just, psql, the repo and a .env, and neither binary migrated on startup. A fresh install had no tables and the API 500'd on every request. Nothing recorded what had been applied, so ordering lived in whoever remembered to add a justfile line. `sqlx::migrate!` embeds each file's SQL at compile time, so the migration set travels inside the binary. There is one version coordinate — the image tag — and no way to deploy code without the migrations it expects. A ConfigMap or a separate migration image would reintroduce two artifacts to keep in lockstep. `app migrate` applies them and exits, so the same image can run as a pre-upgrade Job; the exec-form ENTRYPOINT means `args: ["migrate"]` appends cleanly. INVOKR_DB_MIGRATION_MODE selects the behaviour: none do nothing (default, so no deployment gains behaviour it lacked) run apply pending migrations dry-run print the SQL a run would execute, without applying it Defaulting to none keeps *when* migrations apply a deployment decision, while *what* ships stays coupled to the code. dry-run writes pure SQL to stdout — the summary goes to stderr — so it pipes straight into psql, and is how an existing database gets baselined: keep the _sqlx_migrations INSERTs for migrations already applied by hand, discard the DDL. just db-migrate now runs the same code path rather than a shell loop, so developers exercise the mechanism production uses, and adding a migration no longer means editing the justfile. e2e likewise migrates through the binary, exercising ordering, advisory locking and bookkeeping. The migrations job keeps its psql loop as the fast check that the SQL itself applies, and gains assertions so the invariant is checked rather than printed: public must hold exactly the four control-plane tables, and cron.job must be queryable. The first would have caught the stale txn_based_pickup migration (#77) the day #47 landed. crates/common/migrations/workspace_v1.sql is deliberately not part of the set: it is a runtime template applied per workspace with {p} substituted. A test asserts no embedded migration executes an unsubstituted placeholder, stripping `--` comments first, since #77's no-op quotes the template in its explanation. sqlx stays at 0.7.4 — the migrate feature is already enabled by default, and with #77 turning txn_based_pickup into a no-op nothing needs the 0.8-only `-- no-transaction` escape. Images also gain arm64. The docker job becomes a two-dimensional matrix, arch x image, so it expands to the full 2x4 cross-product, with each architecture built on a native runner (ubuntu-latest, ubuntu-24.04-arm) and pushed to an arch-suffixed tag. A create-manifest job then joins each pair into the version and latest tags. Deliberately not `platforms: linux/amd64,linux/arm64` on one runner: that cross-builds arm64 under QEMU, which for a Rust release build is punishingly slow. The Dockerfile already handled both architectures — its Tailwind download branches on dpkg --print-architecture — so only the workflow needed changing. Cache scopes are per-arch, or each architecture would evict the other's layers on every release. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tsdk02
force-pushed
the
migrations-sqlx
branch
from
September 25, 2026 11:09
0f8b1ac to
affa28f
Compare
This branch has not been deployed
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.
Rebased onto
04082026-releaseafter #66, #46 and #77 merged.#77 overlaps this PR and lands first. It reached the same diagnosis of
20260322000000_txn_based_pickup.sqlindependently and turned the file into adocumented no-op rather than deleting it, because the filename is referenced
from the justfile,
scripts/docker-prod.sh, the README and the docs site. Thatcall stands; this PR rebases onto it and no longer touches the file.
Migrations could not run in a cluster
The SQL never entered the image — the Dockerfile copies only the compiled binary
— the runner was hardcoded
psql -flines in the justfile needingjust,psql, the repo and a.env, and neither binary migrated on startup. A freshinstall had no tables and the API 500'd on every request. Nothing recorded what
had been applied, so ordering lived in whoever remembered to add a justfile line.
sqlx::migrate!embeds each file's SQL at compile time, so the set travelsinside the binary. One version coordinate — the image tag — and no way to deploy
code without the migrations it expects. A ConfigMap or a separate migration image
would reintroduce two artifacts to keep in lockstep.
app migrateapplies them and exits, so the same image can run as a pre-upgradehook Job. The exec-form
ENTRYPOINTmeansargs: ["migrate"]appends cleanly.INVOKR_DB_MIGRATION_MODEnone(default)rundry-runDefaulting to
nonekeeps when migrations apply a deployment decision, whilewhat ships stays coupled to the code.
dry-runwrites pure SQL to stdout — thesummary goes to stderr — so it pipes straight into
psql, and is how an existingdatabase gets baselined.
Dev and CI run the same code path
just db-migratenow invokes the binary rather than a shell loop, so developersexercise the mechanism production uses and adding a migration no longer means
editing the justfile. e2e migrates through the binary too, exercising ordering,
advisory locking and the
_sqlx_migrationsbookkeeping.The
migrationsjob keeps its psql loop as the fast check that the SQL itselfapplies, and gains assertions so the invariant is checked rather than printed:
publicmust hold exactly the four control-plane tables, andcron.jobmust bequeryable. The first assertion would have caught the stale migration the day #47
landed.
workspace_v1.sqlis deliberately not part of the set — it is a runtimetemplate applied per workspace with
{p}substituted. A test asserts no embeddedmigration executes an unsubstituted placeholder; it strips
--comments first,because #77's no-op quotes the template's index definition in its explanation.
sqlx stays at 0.7.4. The
migratefeature is already on by default, and with#77 turning
txn_based_pickupinto a no-op nothing needs the 0.8-only-- no-transactionescape hatch.Images gain arm64
The
dockerjob becomes a two-dimensional matrix —arch×image— so itexpands to the full 2×4 cross-product, each architecture built on a native
runner (
ubuntu-latest,ubuntu-24.04-arm) and pushed to an arch-suffixedtag. A
create-manifestjob then joins each pair into theversionandlatesttags.Deliberately not
platforms: linux/amd64,linux/arm64on one runner — thatcross-builds arm64 under QEMU, which for a Rust release build is punishingly
slow.
The Dockerfile already handled both architectures (its Tailwind download
branches on
dpkg --print-architecture), so only the workflow changed. Cachescopes are per-arch; a shared scope would have each architecture evicting the
other's layers on every release.
Before deploying to an existing database
Production has the schema applied by hand and no
_sqlx_migrationstable, soa first
runwould try to apply everything again.INVOKR_DB_MIGRATION_MODE=dry-runINSERT INTO _sqlx_migrationsstatements it emits — discard the DDLTest plan
Verified against a live pg_cron-enabled database, post-rebase:
_sqlx_migrationspublicholds exactlyorganizations,region_heartbeats,region_status,workspacescron.jobqueryable — pg_cron loaded, not merely createddry-runprints the migrations plus their bookkeeping INSERTs and applies nothingdry-runpiped intopsqlproduces a fully migrated, correctly bookkept databaseruntwice leaves the same rows;dry-runthen reports no pending migrationsmode=nonedoes nothing; an invalid mode is rejected naming the valid valuesGET /health→ 200cargo fmt,cargo clippy --workspace --all-targets,cargo test --workspacecleancreate-manifestproducing 4 multi-arch tags🤖 Generated with Claude Code