-
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
Extension for Flyway #1575 #1649
Conversation
...ration-tests/flyway/src/main/java/io/quarkus/example/flyway/FlywayFunctionalityResource.java
Outdated
Show resolved
Hide resolved
@cristhiank please squash your commits to have an atomic one. Thanks. |
6c86272
to
4da876a
Compare
@gsmet ok, done and pushed 👍 |
4da876a
to
4a0b0b2
Compare
copyright headers should prolly use 2019 by now ;-) |
@fbricon Sorry for the copy-paste on that 😅 |
extensions/flyway/deployment/src/main/java/io/quarkus/flyway/FlywayBuildStep.java
Outdated
Show resolved
Hide resolved
...sions/flyway/runtime/src/main/java/io/quarkus/flyway/runtime/QuarkusPathLocationScanner.java
Outdated
Show resolved
Hide resolved
...sions/flyway/runtime/src/main/java/io/quarkus/flyway/runtime/QuarkusPathLocationScanner.java
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, except for those few minor things I mentioned
...sions/flyway/runtime/src/main/java/io/quarkus/flyway/runtime/QuarkusPathLocationScanner.java
Outdated
Show resolved
Hide resolved
...sions/flyway/runtime/src/main/java/io/quarkus/flyway/runtime/QuarkusPathLocationScanner.java
Outdated
Show resolved
Hide resolved
4a0b0b2
to
22cffdb
Compare
Thanks @stuartwdouglas, I made the changes and pushed again. |
You will also need to rebase this on upstream master, looks like there are conflicts. |
22cffdb
to
3802097
Compare
@stuartwdouglas rebased and pushed again, thanks! |
@stuartwdouglas looks like extension modules naming has changed ? -runtime is removed and -deployment was added to the deployment artifactId ? I already did it in the last commit but don't know if it is ok |
Ok, I just found that it has to be excluded from the integration test pom.xml |
01e92be
to
72d7e46
Compare
right, sorry about that! That has long been the intention but it required a lot more work on all build tools - finally done - so don't worry it will never happen again :) |
Thanks @Sanne, I see a lot of activity in the master branch 😅 |
@cristhiank minor nitpicking: could you move the substitutions to a That would be more consistent with the rest of the code. |
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 it's almost ready. I've pointed out a couple of very minor things.
What about documentation, would you like to include those in this same PR or should we do those later? I'm open to help on that as well.
...time/src/main/java/io/quarkus/flyway/runtime/PostgreSQLSqlStatementBuilderSubstitutions.java
Outdated
Show resolved
Hide resolved
...sions/flyway/runtime/src/main/java/io/quarkus/flyway/runtime/QuarkusPathLocationScanner.java
Outdated
Show resolved
Hide resolved
awesome 👍 thanks |
72d7e46
to
f643064
Compare
@Sanne I pushed the latest changes already rebased with upstream/master . Also added a basic guide/docs, let me know what you think. |
f643064
to
2c8c7cc
Compare
I need some orientation with the deployment/runtime modules concepts. Wich one is supposed to be used by the users ? Both ? I ask this because the integration test I made fails if I change the quarkus-flyway-deployment to quarkus-flyway (runtime module) |
hi @cristhiank , sorry for the delay. To answer your question: yes after the recent changes in our build system, end users should only depend on the "runtime" artifacts. The "deployment" artifact is automatically fetched by the Quarkus build tools extensions, but this relies on metadata that must be included in the runtime artifact. It looks like you're missing the new plugin which produces such metadata; replace the
with e.g.
|
Thanks @Sanne , I knew it was some hidden magic I was missing 😅 I will add it and push again. |
2c8c7cc
to
b0024c8
Compare
Rebased to latest upstream/master and added the missing |
b0024c8
to
56c9769
Compare
hi @cristhiank , there seems to be a problem. I see the scanner picked the script up:
but then it fails to actually load these:
I can have a look as well but it will have to wait a couple of days |
@Sanne thanks, I'm checking it right now. I had to leave earlier this morning but now I'm on it. It is something related to the "-deployment" dependency. It works when I add it. |
It was another typo in the name of the plugin that generates the extension metadata The name in master build pom is Line 693 in f79acb5
|
56c9769
to
b7973b2
Compare
awesome work @cristhiank ! It's merged, this will be available for everyone to enjoy on the next release. |
see #1575