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

OpenAPI: Added support for quarkus.resteasy-reactive.path #19475

Merged
merged 1 commit into from
Aug 19, 2021
Merged

OpenAPI: Added support for quarkus.resteasy-reactive.path #19475

merged 1 commit into from
Aug 19, 2021

Conversation

phillip-kruger
Copy link
Member

Fix #19434

@geoand - I think a better fix would be that RestEasy Reactive also produce a ResteasyJaxrsConfigBuildItem like the RestEasy classic ?

Signed-off-by:Phillip Kruger [email protected]

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 18, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building f319f7f

Status Name Step Test failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 16

@phillip-kruger phillip-kruger added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 18, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Aug 18, 2021

Failing Jobs - Building f319f7f

Status Name Step Test failures Logs Raw logs
Native Tests - Messaging2 Download Maven Repo ⚠️ Check → Logs Raw logs

@ebullient
Copy link
Member

@geoand - I think a better fix would be that RestEasy Reactive also produce a ResteasyJaxrsConfigBuildItem like the RestEasy classic ?

The problem is that the two extensions have zero common classes (including build items). Part of why I moved all of the resteasy stuff into the "resteasy-classic" directory was to really emphasize that even though some of those have "common" in the name.. they aren't common between reactive and classic, only between resteasy server and client.

@ebullient
Copy link
Member

ebullient commented Aug 18, 2021

Amendment: What would have been better, IMO, was for both extensions to use quarkus.rest.path, since they can't be used at the same time anyway. Making all downstream extensions understand two config attributes is.. meh. But that's just my opinion. ;)

@phillip-kruger
Copy link
Member Author

+1 on that

@stuartwdouglas
Copy link
Member

I am also +1 on using quarkus.rest.path

@phillip-kruger
Copy link
Member Author

@stuartwdouglas @ebullient - At the moment we have quarkus.resteasy.path and quarkus.resteasy-reactive.path and we want both of those to also support quarkus.rest.path ? Is that something we want as part of this PR ? Or should I just add support for
quarkus.rest.path here and once it's in RestEasy this will work (still supporting the old properties as well) ?

@stuartwdouglas
Copy link
Member

I would just deprecate quarkus.resteasy-reactive.path and update RR to support quarkus.rest.path. I don't know if I would even bother making quarkus.resteasy-reactive.path work.

@phillip-kruger
Copy link
Member Author

@stuartwdouglas ok cool. I am a bit concerned that the reversed has happened it seems (see this comment: #19434 (comment)) from @ebullient . So I am worried that there was a reason that rest.path was deprecated in favor of the other two and now we are revering it ? Also, is there as shared common module used by both classic and reactive that can host this property ?

@stuartwdouglas
Copy link
Member

Oh ok, reading the issue I assumed that quarkus.rest.path was used by Resteasy Classic, but it appears this is not the case. I think this PR is ok as is then.

@stuartwdouglas stuartwdouglas merged commit af843a0 into quarkusio:main Aug 19, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 19, 2021
@quarkus-bot quarkus-bot bot added this to the 2.3 - main milestone Aug 19, 2021
@phillip-kruger phillip-kruger deleted the openapi-restpath branch August 19, 2021 06:47
@phillip-kruger
Copy link
Member Author

Thanks @stuartwdouglas :)

@gsmet
Copy link
Member

gsmet commented Aug 19, 2021

Yes, I prefer we keep it that way rather than introducing a quarkus.rest prefix out of the blue.

I think this should be backported though if we want to make RESTEasy Reactive a first class citizen in 2.2. Adding the label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting quarkus.resteasy.path not reflected in OpenAPI or Swagger
4 participants