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.

Sign up for GitHub

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

Jump to bottom

Take conditional annotations into account for Request and Response filters #29118

Merged
merged 1 commit into from
Nov 8, 2022
Merged

Take conditional annotations into account for Request and Response filters #29118

merged 1 commit into from
Nov 8, 2022

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Nov 8, 2022

Fixes: #29115

@geoand geoand requested a review from Sgitario November 8, 2022 08:46
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/rest labels Nov 8, 2022
@geoand geoand mentioned this pull request Nov 8, 2022
Closed
Sgitario
Sgitario approved these changes Nov 8, 2022
View reviewed changes
Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 8, 2022
@geoand geoand changed the title Take conditional annotation into account for Request and Response filters Take conditional annotations into account for Request and Response filters Nov 8, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Nov 8, 2022

Failing Jobs - Building 0b512a3

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
✖ JVM Tests - JDK 17 MacOS M1 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 MacOS M1 #

- Failing: integration-tests/mailer integration-tests/smallrye-opentracing

📦 integration-tests/mailer

✖ io.quarkus.it.mailer.MailerTest.sendHtmlEmail - More details - Source on GitHub

java.lang.RuntimeException: java.lang.reflect.InvocationTargetException
	at io.quarkus.test.junit.QuarkusTestExtension.throwBootFailureException(QuarkusTestExtension.java:625)
	at io.quarkus.test.junit.QuarkusTestExtension.interceptTestClassConstructor(QuarkusTestExtension.java:696)

📦 integration-tests/smallrye-opentracing

✖ io.quarkus.it.opentracing.OpenTracingTestCase. line 35 - More details - Source on GitHub

java.lang.IllegalStateException: Could not find a valid Docker environment. Please see logs and check configuration
	at org.testcontainers.dockerclient.DockerClientProviderStrategy.lambda$getFirstValidStrategy$7(DockerClientProviderStrategy.java:256)
	at java.base/java.util.Optional.orElseThrow(Optional.java:403)

@geoand geoand merged commit 12a84de into quarkusio:main Nov 8, 2022
@quarkus-bot quarkus-bot bot added this to the 2.15 - main milestone Nov 8, 2022
@geoand geoand deleted the #29115 branch November 8, 2022 13:31
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Nov 8, 2022
@gsmet gsmet modified the milestones: 2.15 - main, 2.14.1.Final Nov 10, 2022
geoand added a commit to geoand/quarkus that referenced this pull request Nov 23, 2022
@geoand
Disallow conditional annotations and declarative filter annotations o…
53b6171 
…n methods

Although we can make this work in the future, it involves a fair amount of work
for a very small gain, so let's make it explicit for now that this combination
is not allowed.

Follows up on: quarkusio#29118
Relates to: quarkusio#29403
@geoand geoand mentioned this pull request Nov 23, 2022
Merged
geoand added a commit to geoand/quarkus that referenced this pull request Nov 23, 2022
@geoand
Disallow conditional annotations and declarative filter annotations o…
930744b 
…n methods

Although we can make this work in the future, it involves a fair amount of work
for a very small gain, so let's make it explicit for now that this combination
is not allowed.

Follows up on: quarkusio#29118
geoand added a commit that referenced this pull request Nov 23, 2022
@geoand
Merge pull request #29432 from geoand/#29118-followup
af7bf36 
Improve support for conditional bean and declarative filter annotations combination
gsmet pushed a commit to gsmet/quarkus that referenced this pull request Nov 24, 2022
@geoand @gsmet
Disallow conditional annotations and declarative filter annotations o…
99fbddb 
…n methods

Although we can make this work in the future, it involves a fair amount of work
for a very small gain, so let's make it explicit for now that this combination
is not allowed.

Follows up on: quarkusio#29118

(cherry picked from commit 930744b)
@gsmet
Copy link
Member

gsmet commented Dec 14, 2022

After discussion with @rsvoboda, we decided to not backport this one as really it's a new requirement. Please complain loudly if you feel strongly about us backporting it.

Note: if we backport this one, we also need to backport #29432 .

@geoand
Copy link
Contributor Author

geoand commented Dec 14, 2022

That's fine with me. We can always reconcsider if users feel strongly about it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sgitario Sgitario Sgitario approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) area/rest kind/bugfix triage/flaky-test
Projects
None yet
Milestone
2.14.1.Final
Development

Successfully merging this pull request may close these issues.

IfBuildProperty on classes with ServerRequestFilter causes build to fail
3 participants
@geoand @gsmet @Sgitario