-
Notifications
You must be signed in to change notification settings - Fork 3k
Pass app model location to Maven test process #50602
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
Conversation
|
Thanks @oehme! Do you think we could keep the Jackson JSON binding in a separate module, out of the appmodel artifact? E.g. |
This comment has been minimized.
This comment has been minimized.
If I can't depend on Jackson in the appmodel project, then I can't use the binding annotations. This would mean writing a bunch of manual serialization/deserialization code to handle all the different types and polymorphism. And then everyone who currently uses |
b0ca678 to
d8ed192
Compare
This comment has been minimized.
This comment has been minimized.
d8ed192 to
2270268
Compare
|
Fixed the formatting issue above. |
This comment has been minimized.
This comment has been minimized.
|
The failures above are probably due to a merge conflict, I'll fix those up. |
2270268 to
823022f
Compare
|
This should be passing now. |
This comment has been minimized.
This comment has been minimized.
|
All green :) The flaky tests look like the same as on the mainline branch. |
|
Hi there ! |
|
I do have some updates. I have a branch where I'm investigating an alternative serialization/deserialization. The basics seem to be working. I need to test it more properly, which is a work in progress. |
|
Lovely. Thanks for the update! |
|
I opened #51430 for a review and further discussion. |
|
I'll rework this PR to just do those changes, because they'll also be necessary for test distribution. Thank you so much for your other PR! |
|
@oehme the alternative PR was merged. Could you please check whether it works for you? |
823022f to
c834a81
Compare
|
Works great! I've adjusted this PR so it only contains two small changes that are still necessary for Maven. |
devtools/maven/src/main/java/io/quarkus/maven/GenerateCodeMojo.java
Outdated
Show resolved
Hide resolved
c834a81 to
a2ec3bc
Compare
|
@aloubyansky I've further simplified this down to just the app model system property. I managed to solve the deploy path problem a different way on our side by searching through the JSON file for any mentioned file paths and treating those as inputs for the remote test process. |
aloubyansky
left a comment
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.
Thanks a lot @oehme!
|
We should still add a few comments about why we pass that system property. If you like to add them, feel free to do that. Otherwise, I'll add them later. |
a2ec3bc to
c9d0c07
Compare
|
Done :) |
This comment has been minimized.
This comment has been minimized.
c9d0c07 to
9d9f3f4
Compare
|
Rebased my branch to hopefuly get rid of those failures. |
This comment has been minimized.
This comment has been minimized.
|
Looks like these are failing on main too. Can you fix that up, so we can be sure this branch doesn't break anything? |
|
Once #51573 has been merged, we'll rebase this one. |
This aligns the behavior with the Gradle plugin and speeds up startup because we no longer have to search for a POM file to find the workspace location and load the app model. It also makes Quarkus out-of-the-box compatible with Develocity test distribution, because POMs are generally not transferred as inputs to the distributed test workers.
9d9f3f4 to
9a8f8e7
Compare
|
I pushed a rebase to your branch @oehme |
Status for workflow
|
|
Thanks! |
This aligns the behavior with the Gradle plugin and speeds up startup
because we no longer have to search for a POM file to find the workspace
location and load the app model.
It also makes Quarkus out-of-the-box compatible with Develocity test
distribution, because POMs are generally not transferred as inputs to the
distributed test workers.