Add Alembic's check model parity utility to the linting

[chouinar]

Ticket

#198

Changes

Added alembic check to the linter which verifies the SQLAlchemy models are in sync with what is in the DB

Context 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:

[chouinar]

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.

[chouinar]

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.

[lorenyu]

@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?

[lorenyu]

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?

[chouinar]

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

[lorenyu]

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

[chouinar]

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.