Skip to content

Alembic setup#579

Open
chetanr25 wants to merge 3 commits into
fireform-core:developmentfrom
chetanr25:alembic_setup
Open

Alembic setup#579
chetanr25 wants to merge 3 commits into
fireform-core:developmentfrom
chetanr25:alembic_setup

Conversation

@chetanr25

Copy link
Copy Markdown
Collaborator

We are adding more models soon (#544) and the current create_all() approach can only create tables, not alter them. This PR sets up Alembic so we can track and apply schema changes going forward.

Changes

  • Added alembic to requirements.txt
  • Created alembic.ini, alembic/env.py, and alembic/script.py.mako
  • Wrote the first migration (001_initial_schema.py) that captures the existing Template and FormSubmission tables
  • Updated app/db/init_db.py to run alembic upgrade head instead of create_all()
  • Tests still use create_all() with in-memory SQLite for speed
  • Added alembic check step in CI to catch missing migrations
  • Added make migrate and make migration commands

Part of #541
Fix: #578

@chetanr25 chetanr25 marked this pull request as ready for review June 22, 2026 18:22
@abhishek-8081

Copy link
Copy Markdown
Collaborator

Nice setup overall — the initial migration, the round-trip tests, and the CI check are all solid. A few things I think are worth addressing before merge:

init_db.py runs both create_all() and migrations. Right now init_db() calls SQLModel.metadata.create_all(engine) and then run_migrations(). Having both kind of defeats the point of Alembic — create_all builds tables straight from the models, then migrations try to manage the same tables, and the two can drift. I think this should be run_migrations() only, dropping the create_all line, so Alembic is the single source of truth for schema. (This matters for #544 too — once I add the new models, I want Alembic managing them, not create_all creating them behind Alembic's back.)

alembic check in CI + my #544 models. Good call adding the check — but heads up that it means my #544 PR will need to include the migration for the six new models, or this check will fail the build. That's the right behavior, just flagging so we're aligned that the models PR now carries its own migration.

Small one in env.py: from app.models.models import SQLModel — SQLModel is the base class from the sqlmodel package, so this reads a bit confusingly (it works only because models.py re-exposes it). Might be cleaner as from sqlmodel import SQLModel. The Template, FormSubmission import is correct and needed for autogenerate to see them.

One note on the tests: they run against in-memory SQLite, which is great for verifying up/down/round-trip mechanics. Just worth keeping in mind that since the app runs on Postgres, migrations using Postgres-specific types (JSONB, etc. — which the #544 models will use) won't be exercised by these. Not a blocker for this PR, just relevant for what comes next.

Happy to re-review once you've had a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set up Alembic for database migrations

2 participants