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

Use aerich for database migrations #1110

Merged
merged 7 commits into from
Oct 2, 2024

Conversation

bigherc18
Copy link
Contributor

@bigherc18 bigherc18 commented Sep 29, 2024

This PR is about using aerich to add support for database migrations

Here's a demo: https://asciinema.org/a/Sw7OwQZKmWyznV3xgohjd59OT

I added a new config called migrations_dir to specify where migration files should be stored

database:
  migrations_dir: ./migrations

A a set of commands have been added to manage database migrations:

  • dipdup schema history to show the migration history
  • dipdup schema heads to show the show the current available heads
  • dipdup schema migrate to create a new migration changes file
  • dipdup schema upgrade to upgrade the database to the latest or a specified version
  • dipdup schema downgrade to downgrade the database to a previous version

Theses commands invoke aerich's commands under the hood

The migrations directory should be initialized before using any of these commands, either with dipdup schema init or by running the indexer with dipdup run.

Note that aerich commands can't be used directly, as it expects the config to be stored in pyproject.toml

@bigherc18 bigherc18 force-pushed the feat/add-db-migration branch from 1d47481 to 0ec6868 Compare September 29, 2024 03:26
@droserasprout droserasprout added feature New feature or request community Reported or suggested by our awesome users ODHack8 labels Sep 29, 2024
@droserasprout droserasprout linked an issue Sep 29, 2024 that may be closed by this pull request
- Write Changelog and a first draft for the docs
- Abort migration commands if it's not initialized
- Refactor the initialization to `DipDup._initialize_migrations`
- Schema is approved automatically after upgrade and downgrade
- `wipe_schema` also removes the `migration_dir`
@bigherc18 bigherc18 marked this pull request as ready for review September 30, 2024 04:12
Copy link
Member

@droserasprout droserasprout left a comment

Choose a reason for hiding this comment

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

Hi! Thanks for your contribution! It's a great code with many smart solutions; I like every piece of it! My only concern is making this integration optional in every way: dependencies, imports, files in the project package.

src/dipdup/cli.py Outdated Show resolved Hide resolved
src/dipdup/cli.py Outdated Show resolved Hide resolved
src/dipdup/config/__init__.py Outdated Show resolved Hide resolved
pyproject.toml Outdated Show resolved Hide resolved
src/dipdup/cli.py Outdated Show resolved Hide resolved
- aerich is now an optional dependency under the optional migrations group
- migrations folder is hardcoded in the DipDupPackage class
- migrations commands and folder will only be added if aerich is installed
@bigherc18 bigherc18 force-pushed the feat/add-db-migration branch from 0505890 to aa179fc Compare October 1, 2024 13:28
@bigherc18
Copy link
Contributor Author

Do you guys prefer rebase or merge with next to fix the conflicts ?

@droserasprout
Copy link
Member

@bigherc18 We squash every merge request, so it's up to you what's inside.

@bigherc18
Copy link
Contributor Author

bigherc18 commented Oct 1, 2024

Some concerns and limitations I have:

  • aerich will be confused if the user changes the database. If we want to support changing the database, we have to make aerich aware of which database is being used.
  • If you answer "No" to the question "Downgrade is dangerous, which may cause data loss. Are you sure? [y/N]" after running dipdup schema downgrade, the CLI hangs there forever.
  • aerich has confusing messages after running heads and history. It makes the user feel like they have to take action when everything is actually fine: "No available heads, try migrate first."

Copy link
Member

@droserasprout droserasprout left a comment

Choose a reason for hiding this comment

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

Your last comment shows such a great amount of testing done! 🥰 I think, there's nothing critical. Yes, running commands in random order on multiple databases sometimes confuse aerich, but all real cases works perfectly.

I have just a few nitpicks. Then make sure that make all docs command passes, add yourself to the thanks.md file, and let's merge it!

.vscode/settings.json Outdated Show resolved Hide resolved
src/dipdup/database.py Outdated Show resolved Hide resolved
src/dipdup/cli.py Outdated Show resolved Hide resolved
src/dipdup/cli.py Outdated Show resolved Hide resolved
@bigherc18
Copy link
Contributor Author

I think, there's nothing critical. Yes, running commands in random order on multiple databases sometimes confuse aerich, but all real cases works perfectly.

Thank you for your feedback but I just want to make sure I explained my concerns clear enough for you

  1. Multiple databases not supported by migrations

TLDR; if a user uses multiple databases at the same time, migrations is not supported

I see dipdup makes it easy for users to swap databases by just combining different config files with multiple -c args, the problem is: if a user swaps his database after doing some migrations in the first one, aerich is now really confused and I don't even know what will happen, the aerich table will be empty (or contains migrations history for the second database) but the migrations folder still contains files from the first database

I think this worth at least a note in the docs

  1. CLI handing

Here's a little video demonstrating the issue:

demo

@droserasprout droserasprout added this to the 8.1.0 milestone Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community Reported or suggested by our awesome users feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Database migrations with Aerich
2 participants