-
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
Fix RESTEasy Classic GZIP max input in native mode #41054
Fix RESTEasy Classic GZIP max input in native mode #41054
Conversation
the failure is related, but it is also very strange as it is set via |
2db3308
to
0939f4a
Compare
It seems that previous PR added fix but that fix also failed contract established by |
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 some comments inline. I'm not sure I understand what the problem is and what you are trying to test.
...on-tests/resteasy-jackson/src/main/java/io/quarkus/it/resteasy/jackson/GreetingResource.java
Outdated
Show resolved
Hide resolved
...time/src/main/java/io/quarkus/resteasy/runtime/standalone/ResteasyConfigurationMPConfig.java
Show resolved
Hide resolved
...sy-common/runtime/src/main/java/io/quarkus/resteasy/common/runtime/ResteasyCommonConfig.java
Show resolved
Hide resolved
I am starting to think I should had created issue instead of PR. Sorry, it felt like very trivial. I am trying to fix GZIP in a native mode where it doesn't work. (AKA creating issue first is probably better approach to discuss the issue). |
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.
A PR is perfectly fine, don't worry.
I'm good with the PR except for the added test that looks very odd to me. I would refrain from adding this.
We would probably need a test that actually checks the behavior, makes sure we don't have the warning and so on. But I'm not sure it's worth the hassle.
...on-tests/resteasy-jackson/src/main/java/io/quarkus/it/resteasy/jackson/GreetingResource.java
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
0939f4a
to
5d0a210
Compare
Status for workflow
|
Thanks! |
Quarkus QE tried to test #39828 in native mode but it doesn't work because https://github.com/quarkusio/quarkus/blob/main/extensions/resteasy-classic/resteasy/runtime/src/main/java/io/quarkus/resteasy/runtime/standalone/ResteasyConfigurationMPConfig.java is executed during the runtime while https://github.com/quarkusio/quarkus/blob/main/extensions/resteasy-classic/resteasy-common/deployment/src/main/java/io/quarkus/resteasy/common/deployment/ResteasyCommonProcessor.java#L127 is a build-time property. I propose to just convert whole config to the build-time runtime-fixed, but there are more fine-grained approaches possible.
See failure here: https://github.com/quarkus-qe/quarkus-test-suite/actions/runs/9410317682/job/25922118169