-
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
New Route SPI not requiring Vert.x HTTP #38143
Conversation
Thanks @cescoffier! @marko-bekhta could you try rebasing your PR on this one and see if you can sort out the dependencies now? I mean your PR that adds a management interface to Hibernate Search. |
@yrodiere done 😃 I've tried these changes locally and it seems to be working OK, so I've also pushed it to see if the CI will also be happy #35065 |
Unlike the current one, this SPI allows declaring routes without having to depend on Quarkus Vert.x HTTP. Thus, if an extension declares a route with this SPI, and Quarkus Vert.x HTTP is not present, the route is ignored. However, if Quarkus Vert.x HTTP is present, the route is registered.
Failing Jobs - Building 50c898b
Full information is available in the Build summary check run. Failures⚙️ JVM Tests - JDK 21 #- Failing: extensions/scheduler/deployment
! Skipped: extensions/quartz/deployment extensions/spring-scheduled/deployment integration-tests/main and 7 more 📦 extensions/scheduler/deployment✖
|
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 added a question that I want us to sort out before merging.
It might just be me being overcautious :).
|
||
<dependency> | ||
<groupId>io.vertx</groupId> | ||
<artifactId>vertx-web</artifactId> | ||
</dependency> |
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.
Do we really want to add this dependency? The idea would be to not depend on anything Vert.x right?
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 don't know if it's necessary, but as far as I'm concerned, it's fine. A mandatory dependency in the deployment artifact is acceptable as long as it can be optional at runtime (and from what I see in Marko 's PR, it can be).
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.
(to clarify, the initial goal was to make the dependency optional at runtime, though Clément may have gone a bit further here)
ABSOLUTE_ROUTE | ||
} | ||
|
||
private RouteType typeOfRoute = RouteType.APPLICATION_ROUTE; |
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.
Hm, I believe that build items should be immutable. In theory, the build item producer can (even mistakenly) modify the build item through the builder after it's produced and that may lead to unexpected errors.
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're right. I did that for simplicity, but I should use the builder to keep everything and build an immutable route build item.
Unlike the current one, this SPI allows declaring routes without having
to depend on Quarkus Vert.x HTTP. Thus, if an extension declares a route
with this SPI if Quarkus Vert.x HTTP is not present, then the route is ignored.
However, if Quarkus Vert.x HTTP is present, then the route is registered.
I changed the info and openapi extensions to verify that the new SPI is correctly handled.
\CC @yrodiere
Fix #37144.