Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

run alembic migrations instead of using sqlalchemy to migrate test db #169

Merged

Conversation

mduffin95
Copy link
Contributor

@mduffin95 mduffin95 commented Nov 11, 2024

Initial change required for work on #165.

Currently the tests do not seem to test the actual alembic migrations but instead use sqlalchemy directly to build the DB schema. I guess this is problematic because it's not really testing what's in prod.

Because as part of #165 I'd like to use a DB trigger it makes sense to instead apply the alembic migrations so that it can be tested. The existing test setup wouldn't add the trigger.

@mduffin95 mduffin95 marked this pull request as ready for review November 11, 2024 23:14
@mduffin95 mduffin95 changed the title run alembic migrations instead of creating sqlalchemy models directly run alembic migrations instead of using sqlalchemy to migrate test db Nov 11, 2024
@mduffin95
Copy link
Contributor Author

Not sure why the github actions workflow isn't running

@peterdudfield
Copy link
Contributor

Not sure why the github actions workflow isn't running

The CI is currently set up only to run when a PR is opened. I need (or you could if you fancy it) change this so it runs on every change

@mduffin95 mduffin95 closed this Nov 16, 2024
@mduffin95 mduffin95 reopened this Nov 16, 2024
@mduffin95
Copy link
Contributor Author

@peterdudfield I've fixed this PR and also added #171 to add the synchronize trigger.

Copy link
Contributor

@peterdudfield peterdudfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as the tests pass, I'm happy with this.
Thanks @mduffin95 for doing this, let me know if this is ready

@mduffin95
Copy link
Contributor Author

@peterdudfield yep it's ready

@peterdudfield peterdudfield merged commit a477dfb into openclimatefix:main Nov 25, 2024
2 checks passed
@mduffin95 mduffin95 mentioned this pull request Dec 5, 2024
5 tasks
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.

2 participants