Open chouinar opened 11 months ago
Need to look at this a bit more - looks like normal DB migrations don't happen in the CI like the code I'm porting this from.
Need to look at this a bit more - looks like normal DB migrations don't happen in the CI like the code I'm porting this from.
The place I ported this from did the CI a bit different. It ran make init
and then make lint
, but we don't do make init
here. Here, the DB does get started, but only because we're running docker commands.
@lorenyu - thoughts on whether we should adjust something here?
One approach we could do is to add another check that does make init-db db-check-model-parity
to verify the DB is up-to-date. That, or we just add init-db
to the linting step.
@chouinar if i understand correctly, this command checks if the local database has any pending migrations, which doesn't quite strike me as being in the category of "linting", which is more about programming style and conventions and should should be able to be done purely by analyzing source code without any sort of building or initialization.
we could potentially make this a separate job in ci-app rather than as part of the lint job
could you help me understand better what the check is solving? i see the description says "It's very easy to forget to generate your migrations". So is that a scenario where a developer makes a change to the models/schema but forgets to generate migrations?
yeah after thinking about it a bit more, my initial reaction is to make this a separate job that:
thoughts?
yeah after thinking about it a bit more, my initial reaction is to make this a separate job that:
- checks out the repo
- initializes the local db and runs migrations locally
- runs alembic check
thoughts?
So, would that be roughly a new step like this?
test:
name: DB Model Parity
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v3
- name: Start tests
run: |
make init-db
make db-check-model-parity
yep something like that. i would love call it something other than "model parity", since i'm not quite sure what that phrase means, but i also can't come up with anything off the top of my head.
on a related note, are there other common database-related errors that could be worth checking? i vaguely remember either you or someone else suggesting the idea of checking to make sure that no new database migrations were merged to main that aren't in this branch, to prevent the alembic multiple heads / merge issue
yep something like that. i would love call it something other than "model parity", since i'm not quite sure what that phrase means, but i also can't come up with anything off the top of my head.
on a related note, are there other common database-related errors that could be worth checking? i vaguely remember either you or someone else suggesting the idea of checking to make sure that no new database migrations were merged to main that aren't in this branch, to prevent the alembic multiple heads / merge issue
There's a unit test that checks if you'd end up in the multi-head state in the test_migrations file. Since GitHub requires merges from main to merge a PR, that should catch it now.
Ticket
https://github.com/navapbc/template-application-flask/issues/198
Changes
Added
alembic check
to the linter which verifies the SQLAlchemy models are in sync with what is in the DBContext for reviewers
It's very easy to forget to generate your migrations, this will make the linting step tell you.
Testing
If you're already up-to-date, you should get an output like:
If you're out of date (easily done with
make db-migrate-down db-check-model-parity
), you'll get: