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.

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

Add OIDC Redis Token State Manager extension #44546

Conversation

michalvavrik
Copy link
Member

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/docstyle issues related for manual docstyle review area/documentation area/oidc labels Nov 16, 2024
@michalvavrik michalvavrik added area/redis area/documentation dependencies Pull requests that update a dependency file area/docstyle issues related for manual docstyle review and removed area/documentation area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/docstyle issues related for manual docstyle review labels Nov 16, 2024
Copy link

github-actions bot commented Nov 16, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

@michalvavrik michalvavrik requested a review from Ladicek November 16, 2024 20:24

This comment has been minimized.

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.

I've added a small comment about the Redis key name.

Now, I'm wondering about the future. I was planning to extract Redis to a QV repository. I believe this will come with it (as well as the cache implementation). It's not planned for "now", but something for the future.

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, but see the question by @cescoffier

@michalvavrik michalvavrik force-pushed the feature/quarkus-oidc-redis-token-state-manager branch from b58fe91 to 5e27bb4 Compare November 18, 2024 10:10
@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels Nov 18, 2024
@michalvavrik
Copy link
Member Author

michalvavrik commented Nov 18, 2024

Now, I'm wondering about the future. I was planning to extract Redis to a QV repository. I believe this will come with it (as well as the cache implementation). It's not planned for "now", but something for the future.

There is only a couple QV projects I'd use in production. I think you or Sergey needs to decide this because moving it will come with a price. You won't have to just test and release this OIDC Redis extension with every Quarkus release, but also with every QV Redis release. This way, you will have matching tested version for every Quarkus/Redis/OIDC combination. I just checked OIDC Proxy extension in QV and I don't think things are automated in QV side (though I know next to nothing about Quarkiverse). If that will need to be maintained manually, it will be extra work. Maybe you could move it into shared QV project with Redis?

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/quarkus-oidc-redis-token-state-manager branch from 5e27bb4 to c3440a4 Compare November 18, 2024 11:00
Copy link

quarkus-bot bot commented Nov 18, 2024

Status for workflow Quarkus Documentation CI

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

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

Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Thanks @michalvavrik, very neat and simple, thanks @Ladicek for the review

@sberyozkin
Copy link
Member

@calvernaz, FYI

@sberyozkin
Copy link
Member

Hi @michalvavrik @cescoffier
Apologies I missed the conversation about possible moving of Quarkus Redis Client to Quarkiverse. Yeah, we'll need to decide if the OIDC extension which depends on Redis Client will have to follow it and go to Quarkiverse itself, or may be, we should use this opportunity and figure out how Quarkus externsions living in the main repository can work together with Quarkiverse extensions... Otherwise, if this extension also moves, it is unclear if we have it supported at the product level... It will be an interesting discussion :-)

Copy link

quarkus-bot bot commented Nov 18, 2024

Status for workflow Quarkus CI

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

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


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/smallrye-reactive-messaging/deployment

io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

  • Expecting actual: ["-4","-5","-6","-7","-8","-9","-10","-11"] to start with: ["-3", "-4", "-5", "-6"] - java.lang.AssertionError
java.lang.AssertionError: 

Expecting actual:
  ["-4","-5","-6","-7","-8","-9","-10","-11"]
to start with:
  ["-3", "-4", "-5", "-6"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:36)

@sberyozkin sberyozkin merged commit 2b2dbe4 into quarkusio:main Nov 18, 2024
55 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.18 - main milestone Nov 18, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Nov 18, 2024
@michalvavrik michalvavrik deleted the feature/quarkus-oidc-redis-token-state-manager branch November 18, 2024 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/docstyle issues related for manual docstyle review area/documentation area/oidc area/redis dependencies Pull requests that update a dependency file kind/enhancement New feature or request release/noteworthy-feature triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce quarkus-oidc-redis-token-state-manager
4 participants