-
Notifications
You must be signed in to change notification settings - Fork 3k
Make Stork optional for REST Client #47435
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
This comment has been minimized.
This comment has been minimized.
|
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
8a765b7 to
7b7789a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 is great, 👍 for the work.
https://quarkus.io/guides/rest-client doesn't mention Stork at all, so no need to update that one.
I think https://quarkus.io/guides/stork#bootstrapping-the-project and https://quarkus.io/guides/stork-kubernetes#bootstrapping-the-project will need to be updated to include quarkus-smallrye-stork.
https://quarkus.io/guides/stork-reference probably doesn't need any change.
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.
LGTM. But why are the tests on windows failing? Could you please check?
|
@rsvoboda let me check the doc. I had a quick look but apparently not a close enough look. |
If you want to use Stork, you now have to add the quarkus-smallrye-stork extension, which IMHO makes perfect sense. This avoids initializing Stork for every application using the REST Client. Fixes quarkusio#47337
7b7789a to
70ac695
Compare
|
@rsvoboda I adjusted the doc I will also adjust the quickstarts if this gets accepted. |
Status for workflow
|
Status for workflow
|
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.
Makes sense
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.
The code looks good, but we need to make sure the documentation is correct, including the Stork documentation (@aureamunoz, once this is in, we need to change the Quarkus doc on the Stork side).
Note that we are discussing using Stork to handle "addresses" in #46915 - which would make Stork mandatory everywhere.
|
@cescoffier I'm a bit unsure it's a good idea to use Stork (and always have Stork initialized) for such a low level feature. It will have an overhead that doesn't seem very necessary. |
|
@gsmet make this one noteworthy? |
|
ok, it has breaking change label, so it will be in release announce |
|
No changes were necessary in the quickstarts as the Stork modules already depend on |
+1, and Quickstarts Compilation step would detect that |
|
Actually, it might not if Stork is used transparently. The Quickstarts step is just a compilation step, it doesn't run the tests. |
You are right
We can expand to run the tests :) Maybe another |
If you want to use Stork, you now have to add the quarkus-smallrye-stork extension, which IMHO makes perfect sense.
This avoids initializing Stork for every application using the REST Client.
I tested several ITs both in JVM and native and it looked fine. Let's hope CI will agree.
Fixes #47337