-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Flyway Build Item (prerequisite for multitenant extension) #43892
base: main
Are you sure you want to change the base?
Conversation
rmanibus
commented
Oct 16, 2024
•
edited
Loading
edited
- Create Flyway build item to allow other extensions to create flyway instances
- Allow multiple flyway instances by datasources (add unique id attribute that is not the ds name)
- move startAction in flyway container to allow other extension to define different behavior
77c0ef9
to
9b18495
Compare
cc @geoand |
9b18495
to
7fbb1cc
Compare
This comment has been minimized.
This comment has been minimized.
🎊 PR Preview b46c8ef has been successfully built and deployed to https://quarkus-pr-main-43892-preview.surge.sh/version/main/guides/
|
This comment has been minimized.
This comment has been minimized.
Nice! I'll have a close look in the following days, but I see that CI is not happy.
|
7fbb1cc
to
d3e33bb
Compare
4604dea
to
8758112
Compare
This comment has been minimized.
This comment has been minimized.
8758112
to
9a47185
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
9a47185
to
5c8b85a
Compare
Status for workflow
|
|
||
import java.util.List; | ||
|
||
public interface FlywayTenantSupport { |
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.
not sure about the naming of this one
This comment has been minimized.
This comment has been minimized.
Not sure a separate extension for this is necessary: It looks like this feature can be implemented in the |
That's a good idea! |
Conditional dependencies might also make sense |
I will try to explore this option. It does not feel right to mix flyway instances tied to data sources and others tied to persistence units |
I'll defer this one to @yrodiere who has thought a lot about Datasources and their configuration |
Yes, that makes sense |
Not a fan of the approach either. I'd much rather we fixed the root of the problem: #11861 But, well. Someone needs to work on #11861. |
Well , I could have a look into this one too. Until this is done, since tenancy has been coupled with PU forever, could it be ok to still have flyway instances coupled to PU as introduced in this PR in an entirely different namespace (this PR uses quarkus.flyway.multitenant, but it could also be quarkus.flyway.hibernate) ? |
That would be great, thank you!
Adding something like this to Quarkiverse is always an option, for sure. To the core repo... I doubt it. For something urgent and temporary, Quarkiverse would be better IMO. But then it's not like my approval is needed to add extensions to Quarkus :) |
sure, but it would ideally still requires some change in the core repo, mainly to expose the flyway build item that does not exist today |
should I reduce the scope of this PR to the changes introduced in the main flyway extension and then move everything from the flyway-multitenant in the quarkiverse ? |
Fine by me. @geoand ? |
+1 |
5c8b85a
to
7aa00b9
Compare
7aa00b9
to
0838b2c
Compare
@geoand I cleaned up this PR, what do you think ? |
0838b2c
to
15427c8
Compare
Seeing the complexity of the build item, I am wondering if it is a good idea to try to reuse the logic from the flyway extension. It might be simpler just to create the multi tenancy extension from scratch |
I will take a better look at your changes next week to see what's involved and let you know |
import io.quarkus.flyway.runtime.FlywayContainerProducer; | ||
import io.quarkus.flyway.runtime.FlywayDataSourceBuildTimeConfig; | ||
|
||
public final class FlywayBuildItem extends MultiBuildItem { |
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 would very much like to have some Javadoc on 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.
This seems pretty interesting.
I would like to have some Javadoc on what exactly the FlywayBuildItem
does and what its fields are used for.
Furthermore, it would be great to have a QuarkusUnitTest
that tests the use of the build items using QuarkusUnitTest#addBuildChainCustomizer
(see various examples in the codebase)