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

ArC - support multiple interceptor methods in a class hierarchy and around invoke method declared on target class #32730

Merged
merged 5 commits into from
Apr 20, 2023
Merged

ArC - support multiple interceptor methods in a class hierarchy and around invoke method declared on target class #32730

merged 5 commits into from
Apr 20, 2023

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Apr 18, 2023

No description provided.

@mkouba mkouba force-pushed the arc-interceptors-subclasses branch from 0b7189f to 9bde249 Compare April 18, 2023 14:58
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Apr 18, 2023
@mkouba mkouba force-pushed the arc-interceptors-subclasses branch 2 times, most recently from 6237425 to b0c3c32 Compare April 19, 2023 07:30
@mkouba mkouba marked this pull request as ready for review April 19, 2023 07:33
@mkouba mkouba requested review from Ladicek and manovotn April 19, 2023 07:33
@mkouba mkouba force-pushed the arc-interceptors-subclasses branch from b0c3c32 to ef52dab Compare April 19, 2023 09:09
@Ladicek Ladicek force-pushed the arc-interceptors-subclasses branch from 5d78c5e to e9a489d Compare April 19, 2023 10:29
@mkouba mkouba added this to the 3.1 - main milestone Apr 19, 2023
@mkouba mkouba requested a review from Ladicek April 19, 2023 14:20
Ladicek
Ladicek approved these changes Apr 19, 2023
View reviewed changes
Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

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

LGTM

@Ladicek
Copy link
Contributor

Ladicek commented Apr 19, 2023

I also just noticed that this PR seems to fix PostConstructOrderTest as well. Could you please verify and if so, remove it from the exclusion list? Thanks!

@Ladicek Ladicek mentioned this pull request Apr 19, 2023
Closed
manovotn
manovotn approved these changes Apr 19, 2023
View reviewed changes
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

Took me a solid while to understand (and I am sure to forget that by tomorrow morning) but it looks really good. Thanks @mkouba!

@quarkus-bot

This comment has been minimized.

Sign in to view
mkouba and others added 3 commits April 20, 2023 09:12
@mkouba
ArC TCK runner - update exlude list
f9bfadb 
- removed AroundConstructOrderTest, AroundInvokeOrderTest and PostConstructOrderTest
@Ladicek @mkouba
ArC: improve javadoc for InvocationContext implementations
f181c27
@Ladicek @mkouba
ArC: add tests for business method interceptors on target classes and…
1adf1b7 
… fix some issues

Co-authored-by: Martin Kouba <[email protected]>
@mkouba
Copy link
Contributor Author

mkouba commented Apr 20, 2023

I also just noticed that this PR seems to fix PostConstructOrderTest as well. Could you please verify and if so, remove it from the exclusion list? Thanks!

It does. TBH I did not walk through the entire exclude list so there might be other fixed tests...

@mkouba mkouba force-pushed the arc-interceptors-subclasses branch from 0fcd49a to 1adf1b7 Compare April 20, 2023 07:18
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 20, 2023
@quarkus-bot

This comment has been minimized.

Sign in to view
@Ladicek
Copy link
Contributor

Ladicek commented Apr 20, 2023

VertxMDCTest is known to be flaky, see e.g. #32488. Gonna merge this.

@Ladicek
Copy link
Contributor

Ladicek commented Apr 20, 2023

Or not, I see someone has triggered a rerun. Let's wait for that.

@mkouba
Copy link
Contributor Author

mkouba commented Apr 20, 2023

Or not, I see someone has triggered a rerun. Let's wait for that.

That was me. Sorry..

@Ladicek
Copy link
Contributor

Ladicek commented Apr 20, 2023

No worries!

@quarkus-bot
Copy link

quarkus-bot bot commented Apr 20, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@manovotn manovotn merged commit 3fe819c into quarkusio:main Apr 20, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 20, 2023
@manovotn manovotn mentioned this pull request May 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manovotn manovotn manovotn approved these changes

@Ladicek Ladicek Ladicek approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection)
Projects
None yet
Milestone
3.1.0.CR1
Development

Successfully merging this pull request may close these issues.
3 participants
@mkouba @Ladicek @manovotn