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

Initialize the database automatically when starting mquery #421

Merged
merged 4 commits into from
Oct 15, 2024

Conversation

MichalJura
Copy link
Collaborator

Your checklist for this pull request

  • I've read the contributing guideline.
  • I've tested my changes by building and running mquery, and testing changed functionality (if applicable)
  • I've added automated tests for my change (if applicable, optional)
  • I've updated documentation to reflect my change (if applicable)

What is the current behaviour?

Currently we needed to initilize database by hand: python3 -m mquery.db.

What is the new behaviour?

Now the database initialization is automatic and it uses alembic upgrade head instad of mquery.db so it also applies migrations. Also if the database already exists it will stamp it and create alembic_version for future migrations to be applied via upgrade head

Test plan

For new instance: Create new instance and it will initilize database automatically.
For existing database: Remove (unnecessary for dev containers) web container and set it up again.
Then you can check if there is alembic_version in postgresql.

Closing issues

fixes #402

@MichalJura MichalJura requested a review from msm-cert October 11, 2024 13:46
@MichalJura MichalJura self-assigned this Oct 11, 2024
Copy link
Member

@msm-cert msm-cert left a comment

Choose a reason for hiding this comment

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

This looks more complicated than I thought it would. I'll take a look at it and check if it can be simplified.

@MichalJura
Copy link
Collaborator Author

This looks more complicated than I thought it would. I'll take a look at it and check if it can be simplified.

The reason behind it is that we don't have alembic_version table.
So for new instances there is no problem as alembic upgrade head will just apply migrations and add alembic_version to the database.
But if there is already a database alemibc head upgrade will result in error in Postrgres.
We could simply check if the database is initilized and then pass, but the new migrations like #412 would have to be applied manually.
On the other hand we migth want to apply them manually(but I don't think we are adding some breaking changes to the database schema?).
But in that case we could remove the stamping part.

@msm-cert
Copy link
Member

msm-cert commented Oct 15, 2024

The reason behind it is that we don't have alembic_version table.

We should, otherwise how would previous migrations be applied?

If previous documentation was giving incorrect instructions that bypass alembic it's unfortunate, but we don't have to accomodate such weird cases (database was not a part of any released version so it's not a critical problem).

Sorry if that was not clear and you had to solve this problem unnecessarily.

@msm-cert msm-cert force-pushed the feat/init-database-automatically-402 branch from 3fe6cd8 to fa0510b Compare October 15, 2024 01:10
@msm-cert
Copy link
Member

msm-cert commented Oct 15, 2024

I still believe the previous version was severely overcomplicated. Pushed my suggestions/fixes.

One important note:
image

First thing is, chdir is extremely "dirty" thing to do, and in 99.9% of programs should never be called (it changes process' global state and may cause unpredictable problems). But even more importantly, this place is not guaranteed to exist. It only exists in the current docker environment, but the code should work outside of docker too (many deployments work this way). In this case, code is in a package, and we should just package alembic.ini and migrations.

@msm-cert msm-cert merged commit 33cd906 into master Oct 15, 2024
10 checks passed
@msm-cert msm-cert deleted the feat/init-database-automatically-402 branch October 15, 2024 01:16
@msm-cert msm-cert changed the title Initialize the database automatically when starting mquery #402 Initialize the database automatically when starting mquery Oct 15, 2024
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.

Initialize the database automatically when starting mquery
3 participants