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

Set up alembic migration usage #954

Merged
merged 9 commits into from Feb 21, 2022

Conversation

PFischbeck
Copy link
Contributor

This is a WIP of setting up Alembic for migrations, as suggested in #551.

How this works: Any time you change the DB models, you need to run poetry run alembic revision --autogenerate -m "Describe changes here". Anytime init_db.py is called, all alembic migrations are run if the database is not up-to-date.
I had to change a few imports in order to allow for alembic to find all models and have no circular dependencies.

Some thoughts of what's left to do:

  • Maybe check whether migrations are necessary, see this suggestion in the alembic docs
  • Maybe the initial migration should only add all tables if they don't already exist, so that all devs on mealie-next don't need to delete their database.
  • We could think about using data migrations for the current seeding in init_db.py. The question basically is: what should happen if the seed data changes? Who should benefit from these changes, and how can we achieve that?
  • The step for adding migrations should be documented somewhere
  • We might want to add a test that ensures all upgrades are actually applicable
  • We might want to add a CI test that ensures the migrations result in the current schema, see this discussion.

I only just found #357, so I'm not sure if I have missed anything.

@hay-kot
Copy link
Collaborator

hay-kot commented Feb 3, 2022

This is excellent! Thanks for putting this together.

Maybe check whether migrations are necessary, see this suggestion in the alembic docs

I agree. I think at least doing some basic logging of "Migration needed performing migration" / "Migration not needed" would be sufficient for now.

Maybe the initial migration should only add all tables if they don't already exist, so that all devs on mealie-next don't need to delete their database.

I think that's a common pattern for migrations, create if not exist, is fairly common.

We could think about using data migrations for the current seeding in init_db.py. The question basically is: what should happen if the seed data changes? Who should benefit from these changes, and how can we achieve that?

I say we leave it for now, and cross that bridge later. Getting the initial migrations working with the existing setup is probably a better initial scope. I think we may be able to address this in another user-interactive way on the Frontend.

The step for adding migrations should be documented somewhere
We might want to add a test that ensures all upgrades are actually applicable
We might want to add a CI test that ensures the migrations result in the current schema, see sqlalchemy/alembic#724.

Yes. Yes and Yes! ✅ I don't think this is all strictly needed as apart of this PR. You can get the initial framework setup and then we can build on-top of that in future iterations and track progress in #551

In Summary - I think to get this merged I'd like to see at least some stubs setup for tests that are just marked as skip for now. That way when you, I or someone else want's to get this up to speed we have a clear idea of what needs to get tested. - What else did you want to tackle as apart of this before it gets merged?


The main issue with the other PR is that it was pretty big and touched a lot of pieces of code when we didn't have many tests and the originator didn't write any tests for the work they did. Additionally, I couldn't reproduce his success on my machine so I ended up pulling out some of the parts and adding them into the main branch at the time.

@PFischbeck
Copy link
Contributor Author

I've udpated the PR, including test stubs, and I think the rest can be done in other PRs:

  • Document how to use alembic (but where?)
  • Implement tests

We should wait with merging until you have finished #980, because that PR changes the DB schema quite a lot. After #980 is merged, I will have to update the initial_tables revision via alembic autogenerate, plus re-adding the "only update if tables don't exist yet" part.

@hay-kot
Copy link
Collaborator

hay-kot commented Feb 20, 2022

PR #980 has been merged and I'm done with database changes until we setup alembic, so we can semi-officially put out a v1 Beta release. Let me know if you need anything else from me to get this ready-to merge.

Can't thank you enough for getting this going, if you're ever in Alaska I owe you a few beers 👍

@PFischbeck PFischbeck marked this pull request as ready for review February 21, 2022 19:43
@PFischbeck
Copy link
Contributor Author

I think this is ready to merge 😄

I'll tell you if I'm ever in Alaska, but I owe you a few beers for creating Mealie!

@hay-kot hay-kot merged commit fdfb5b1 into mealie-recipes:mealie-next Feb 21, 2022
@PFischbeck PFischbeck deleted the alembic branch February 21, 2022 20:47
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.

None yet

2 participants