Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[WIP] Refactor jdbc migration and use service loader #151
base: master
Are you sure you want to change the base?
[WIP] Refactor jdbc migration and use service loader #151
Changes from 4 commits
13f69b0
4180c7b
4c0e7cf
33a356c
cb73cd9
58d893f
452a16e
ee3eeb6
f51f437
fbf4e5f
a405ce0
bc51a45
a66a434
16aee17
e60e475
4da3958
2ab8ef2
40d6a73
3de01dc
6d14cf8
4391093
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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 causes an API break.
I could avoid this by "default" methods, but then I think no one reads the changelog and the java migrations will not work anymore.
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.
Ok. Yes I agree that people generally don't read release notes.
This file was deleted.
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.
We have a name mismatch in that
getJdbcMigrationFactory()
returns a "collection of migrations".So I get it but it seems off. Migrations need to be sorted into version order etc. This doesn't quite look right yet.
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.
agree, I'll rename it to
getJdbcMigrationCollection
I don't get you here. They are sorted later here https://github.com/ebean-orm/ebean-migration/pull/151/files#diff-10ad52c1950c7a3f96d7f7286cee908ff73e79324b8414c53ce3ad17204ca9e1R57 and they have to be sorted between the SQL migrations
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'm not sure about service loading JdbcMigration (each migration). I have in my mind that it would be better to add 1 level of indirection (a JdbcMigrationSupplier or JdbcMigrationFactory) ... then typically only that 1 supplier/factory would need to be registered for service loading rather than each JdbcMigration.
Hmmm.
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.
You have the first level of indirection in the migrationConfig. You can set your jdbcMigrationFactory/Collection by a property. So you can implement your own search strategy (we did this with our spring-classpath scan)
I think, just Service-loading the factory is not a good idea, if you have multiple ebean-servers to different databases with different schemas or even different platforms. So the jdbcMigrations have to be configured the same way as the jdbcMigration-directory