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: introduce CurrentManagedContext, built-in session context, @ActivateSessionContext #45170

Merged
merged 4 commits into from
Dec 17, 2024
Merged

ArC: introduce CurrentManagedContext, built-in session context, @ActivateSessionContext #45170

merged 4 commits into from
Dec 17, 2024

Conversation

mkouba
Copy link
Contributor

@mkouba mkouba commented Dec 17, 2024

@mkouba mkouba requested a review from manovotn December 17, 2024 11:56
@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/documentation area/testing labels Dec 17, 2024
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@mkouba mkouba requested a review from cescoffier December 17, 2024 12:14
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 17, 2024 •
edited
Loading

🙈 The PR is closed and the preview is expired.

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
manovotn
manovotn reviewed Dec 17, 2024
View reviewed changes
test-framework/common/src/main/java/io/quarkus/test/ActivateSessionContext.java Outdated Show resolved Hide resolved
test-framework/common/src/main/java/io/quarkus/test/ActivateSessionContext.java
* still terminate when the invocation completes and not at the time the asynchronous type is completed. Also note that session
* context is not propagated by MicroProfile Context Propagation.
* <p>
* This interceptor binding is only available in tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be more convenient to have this in ArC itself with the documentation that it's intended for test usage only?
The context now exists there anyway and you can easily perform the activation anyway. Besides, it would remove some of the registration complexity that you now have in ArcTestSteps. I do not mind the current setup, I just think it'd be easier this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, "intended for test" is not the same as "not available outside tests". I mean, for now I'd like to keep it for tests only because it doesn't make a lot of sense elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

With this PR we already expose Arc.container().sessionContext() so the 'misuse' is very much possible already.
But let's keep it as-is for now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ofc, it is possible but we don't want to make it even easier...

@mkouba mkouba requested a review from manovotn December 17, 2024 15:05
cescoffier
cescoffier reviewed Dec 17, 2024
View reviewed changes
Copy link
Member

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

LGTM - but not an expert at all (I'm on the user side :-))

@mkouba
Copy link
Contributor Author

mkouba commented Dec 17, 2024

LGTM - but not an expert at all (I'm on the user side :-))

It's always good to have feedback from the user side ;-)

manovotn
manovotn approved these changes Dec 17, 2024
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.

Added one small note, otherwise LGTM 👍

...ons/arc/deployment/src/test/java/io/quarkus/arc/test/context/session/SessionContextTest.java Show resolved Hide resolved
@mkouba @manovotn
ArC: introduce the ActivateSessionContext interceptor binding
09706b9 
- it's only available in tests
- fixes quarkusio#45146

Co-authored-by: Matej Novotny <matej.novotny2@gmail.com>
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Dec 17, 2024
@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Dec 17, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 09706b9.

✅ 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.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Dec 17, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 09706b9.

✅ 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.

You can consult the Develocity build scans.

@mkouba mkouba merged commit 9943ebd into quarkusio:main Dec 17, 2024
55 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Dec 17, 2024
@quarkus-bot quarkus-bot bot added kind/enhancement New feature or request and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Dec 17, 2024
@jedla97 jedla97 mentioned this pull request Dec 18, 2024
Merged
9 tasks
@jedla97 jedla97 mentioned this pull request Dec 18, 2024
Closed
@mkouba mkouba mentioned this pull request Jan 13, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cescoffier cescoffier cescoffier left review comments

@manovotn manovotn manovotn approved these changes

Assignees
No one assigned
Labels
area/arc Issue related to ARC (dependency injection) area/documentation area/testing kind/enhancement New feature or request
Projects
None yet
Milestone
3.18.0.CR1
Development

Successfully merging this pull request may close these issues.

ArC: make it possible to activate the session context for a specific business method in a test
3 participants
@mkouba @cescoffier @manovotn