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

Enable migration generation in modules #933

Merged
merged 3 commits into from
Sep 19, 2022
Merged

Enable migration generation in modules #933

merged 3 commits into from
Sep 19, 2022

Conversation

senekor
Copy link
Contributor

@senekor senekor commented Aug 4, 2022

Adds

Generating a new migration no works even if migrations are not at the root of their own crate.

I didn't want to have an entire separate crate just for migrations. So I put them into a module in my regular crate. sea-orm-cli migrate generate accepts a flag --migration-dir, which seemed perfect for my use case. However, that flag still assumes there is a directory src/ and that the migrator definition is in lib.rs.

If there is a directory src/ and a file lib.rs, this PR doesn't change anything, so I don't think it's a breaking change. If src/ doesn't exist, it will look in the parent directory instead. If there is no lib.rs, it will look for a mod.rs instead.

For reference, this is what my file structure looks like:

my-app/
├─ src/
│  ├─ db/
│  │  ├─ migration/
│  │  │  ├─ mod.rs/
│  │  │  ├─ m20220101_000001_create_table.rs/
│  │  ├─ migration_cli.rs
│  │  ├─ mod.rs
│  ├─ main.rs
├─ Cargo.toml

Both db/mod.rs and db/migration_cli.rs contain mod migration;. That way, both binaries (main.rs, the main app, and migration_cli.rs, a thin wrapper around the migration cli api) can access my migrations.

I'm looking forward to discussions and suggestions!

Previously, migration generation expected migrations
to be at the crate root.
Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Hey @remlse, welcome! And thanks for the great contributions!

I think we should document the updated behaviour on the get_full_migration_dir and get_migrator_filepath method because the logic is getter more complex it's not easy to understand at a glance.

Which directory/file be checked for existence then what's the fallback?

Thoughts? :)

Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Hey @remlse, just notice a problem. Before updating the migrator file mod.rs/lib.rs. A backup file will be created with file extension of .bak.

However, now that migrator can be either reside in mod.rs/lib.rs. We should change the line to:

let migrator_backup_filepath = migrator_filepath.with_extension("rs.bak");

@senekor
Copy link
Contributor Author

senekor commented Aug 11, 2022

@billy1624 thanks for having a look! I think I'll get around to it on the weekend, I'll add some documentation and fix the backup filename.

@senekor
Copy link
Contributor Author

senekor commented Aug 13, 2022

@billy1624 I've added some documentation. Some internally and some to be displayed by sea-orm-cli migrate -h. Let me know if I can improve something, I haven't done much documentation before.

@senekor senekor requested a review from billy1624 August 13, 2022 16:12
Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

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

Thanks! @remlse The documentation looks great :)

@billy1624 billy1624 requested a review from tyt2y3 August 16, 2022 09:32
@tyt2y3 tyt2y3 merged commit be0d846 into SeaQL:master Sep 19, 2022
@tyt2y3
Copy link
Member

tyt2y3 commented Sep 19, 2022

Thank you for the contribution. Too bad for the delay, I am surprised this patch still applies

tyt2y3 pushed a commit that referenced this pull request Sep 30, 2022
* Enable migration generation in modules

Previously, migration generation expected migrations
to be at the crate root.

* Fix migration backup file extension

* Document behavior of migration_dir
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants