chore: baseline drizzle migrations against production - #1395
Merged
Conversation
Alembic still owns the schema; this only gives drizzle-kit somewhere to read it from. Nothing generates or applies a migration yet. `schema` points at the barrel rather than a glob, so a migration cannot describe something `createDb` doesn't type the runtime handle against. `migrations` pins kit's own defaults because production's bookkeeping row is stamped by hand later, and a moved default would orphan that stamp. `dbCredentials.url` falls back to an empty string rather than throwing, since `generate` never connects. `out` is excluded from biome: drizzle-kit writes its journal and snapshots with 2-space indent, which would fail `pnpm check` on the first generate.
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider separating config for migration-related commands from the
generateuse case so you can enforce a non-emptyVT_POSTGRES_URLfordb:migratewhile still allowinggenerateto run with a dummy URL. - You might want to centralize the drizzle output directory path (currently
./drizzle) in a shared constant or config, to avoid future mismatches between this file and any tooling/scripts that consume generated artifacts.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider separating config for migration-related commands from the `generate` use case so you can enforce a non-empty `VT_POSTGRES_URL` for `db:migrate` while still allowing `generate` to run with a dummy URL.
- You might want to centralize the drizzle output directory path (currently `./drizzle`) in a shared constant or config, to avoid future mismatches between this file and any tooling/scripts that consume generated artifacts.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Production's 54 foreign keys carry Postgres's default name,
`{table}_{column}_fkey`, because Alembic never named them itself. Inline
`.references()` auto-names a constraint `{table}_{col}_{ref}_{refcol}_fk`,
agreeing with production on the columns, the referenced table and both
referential actions, and disagreeing on all 54 names.
Nothing would catch that at apply time. Migration `0000` is stamped as
already-applied rather than run, so a wrong name reaches no database — it
reaches the snapshot every later generate diffs against, and the first
migration to touch a foreign key emits SQL naming a constraint production
does not have.
`foreignKeys.test.ts` derives the expected name for every foreign key in
the barrel and pins the count, so a table declared with `.references()`
fails the suite by name.
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.
Summary
packages/data/drizzle.config.tsand the generated baseline migration0000_baseline.sql, the first steps of taking Postgres migrations over from Alembic. Alembic still owns the schema — nothing here applies a migration, and no behaviour changes.{table}_{column}_fkey, because Alembic never named these itself; inline.references()auto-names them{table}_{col}_{ref}_{refcol}_fk, agreeing with production on columns, referenced table and both referential actions, and disagreeing on every name.foreignKeys.test.tspins the rule so a table declared with.references()fails by name.Why the naming matters:
0000is stamped as already-applied rather than run, so a wrong name reaches no database. It reachesmeta/0000_snapshot.json, which every latergeneratediffs against — and the first migration to touch a foreign key would emit SQL naming a constraint production does not have.Two divergences are known and tracked rather than fixed here: 89 unbounded
varcharcolumns aretextin the mirror andlegacy_history_diff's sequence keeps its pre-rename name (VIR-2986), and theinstance_messagestrigger has no Drizzle representation (VIR-2987). All three are inert while0000is stamped rather than run.