-
Notifications
You must be signed in to change notification settings - Fork 696
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
feat: Add migrate function #2043
base: main
Are you sure you want to change the base?
Conversation
624af92
to
cf247fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome. We are excited to see this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the API's good so far, just the comments. Might also be worth adding a test for a failed migration, if it's possible to simulate that and check the logs +/- query the schema history table.
* Applies a database migration from [oldVersion] to [newVersion]. | ||
* | ||
* If a migration script with the same name already exists, the existing one will be used as is and a new one will | ||
* not be generated. This allows you to generate a migration script before the migration and modify it manually if | ||
* needed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three things I thought of:
- It's just my opinion, but I think it should be made clear that the migration is being performed using Flyway. Even if the user ultimately doesn't care, at least we're being fully transparent by including it in the KDocs.
- I'd specify how exactly to generate the migration script without performing the migration (at least the name of the function). Last line might be confusing if you don't know that
generateMigrationScript
already exists. - When KDocs are written this way (with params first & summary last), the purpose of the function is tacked on to the bottom, as part of the 'Throw' section. So mousing over or using quick docs hot keys means the user would have to scroll for the summary and might miss it. It's the same for previous migration functions.
* This function uses the Flyway naming convention when generating the migration script. | ||
* If a migration script with the same name already exists, its content will be overwritten. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the Flyway overload, maybe the function name could be different to make it's purpose more obvious. Or at least the line that differentiates it from the other function should be in the summary so it's more obvious. And the KDocs changed so this paragraph isn't added to the 'Throws' section.
...d-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/ddl/DatabaseMigrationTests.kt
Outdated
Show resolved
Hide resolved
...d-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/ddl/DatabaseMigrationTests.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for me! The only concern I have - to move it in a separate module. Let's think how we can do this
cf247fa
to
62cd4d2
Compare
62cd4d2
to
d620f76
Compare
Requesting early feedback for the
migrate
API using Flyway.The database migration is currently failing for SQLite and MariaDB and needs further investigation. For PostgreSQLNG, it does not work with Flyway without using a DataSource.