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

Fix support java migrations with index #149

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rPraml
Copy link
Contributor

@rPraml rPraml commented Nov 22, 2023

Hello Rob,
with #91 the ebean-migration does not scan the classpath for java migrations. There are currently 2 issues

  1. When Ebean generates migrations, it does not add java migrations to the idx file (PR will follow)
  2. Ebean-migration does not support java migrations in the idx file.

This PR adds java-migration support for idx files.
Furthermore, this change may make #90 obsolete (the java class to use is defined in the idx file now)

@rbygrave
Copy link
Member

rbygrave commented Nov 22, 2023

Well, as I see it the right way to find and initiate Java Migrations long term is to use ServiceLoader. The way that Java Migrations work currently as I see it is pretty horrible relative to using a ServiceLoader [with class path scanning and reflection used to construct instances etc].

In that sense, this PR keeps the current Java Migrations working with the index file ... but for me this would only be a temporary solution and that the proper solution is to actually change to use ServiceLoader.

Yes, currently applications that use the existing Java Migrations API can't use the index file. I need to get a sense of what the long term solution looks like using ServiceLoader and then see what that would mean for existing Java migrations etc.

@rPraml
Copy link
Contributor Author

rPraml commented Nov 22, 2023

Hello Rob,

then wait with merging this PR.
I have some ideas how to implement this, as I do not like the hacks that we have to do in ebean.

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.

2 participants