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 first version of Keycloak admin client based on Reactive REST Client #24294

Closed
wants to merge 2 commits into from

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Mar 14, 2022

This is pretty much a straight copy of the existing Keycloak code
that removes various things that are not supported by Quarkus.

Furthermore, the base package was changed because there are cases
where the upstream Keycloak admin client still needs to be used (for example
in a QuarkusTestResourceLifecycleManager which runs before the Quarkus
application and therefore cannot use any Quarkus infrastructure. This case
is exhibited by the second commit, which uses the upstream admin client for the
QuarkusTestResourceLifecycleManager, but the new admin client for the application
itself).

Fixes: #20505

@geoand
Copy link
Contributor Author

geoand commented Mar 14, 2022

geoand added 2 commits March 14, 2022 11:41
This is pretty much a straight copy of the existing Keycloak code
that removes various things that are not supported by Quarkus.

Furthermore, the base package was changed because there are cases
where the upstream Keycloak client still needs to be used (for example
in a QuarkusTestResourceLifecycleManager which runs before the Quarkus
application and therefore cannot use any Quarkus infrastructure)

Fixes: quarkusio#20505
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 14, 2022

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 99e57f3

Status Name Step Failures Logs Raw logs
Initial JDK 11 Build Verify extension dependencies ⚠️ Check → Logs Raw logs

⚠️ Errors occurred while downloading the build reports. This report is incomplete.

@sberyozkin
Copy link
Member

@geoand Hi, that was super fast, thanks. I recall there was some discussion about keeping the actual Keycloak admin client source in the independent-projects, should it be placed there ?
Right now it is a bit hard to see how we can try to start syncing up with at the SPI level

@geoand
Copy link
Contributor Author

geoand commented Mar 14, 2022

I recall there was some discussion about keeping the actual Keycloak admin client source in the independent-projects, should it be placed there ?

We can certainly do that, I don't mind.
The reason I didn't do this is because I think that all this copied code will simply disappear when we update the upstream client, so we won't have to maintain any resources on our side - just an SPI implementation.

@sberyozkin
Copy link
Member

sberyozkin commented Mar 14, 2022

Thanks @geoand, I believe @cescoffier was proposing it. I guess it can be easier for us to see some initial borders between the actual quarkus extension and the admin client source if this client source were located in independent-projects... I propose to move it there if there will be no objections

@geoand
Copy link
Contributor Author

geoand commented Mar 14, 2022

Let's see what @cescoffier says

@cescoffier
Copy link
Member

I agree.

I quickly looked at the pr and the naming is somewhat confusing (reactive but not a reactive api). So we would need to be smart on this.

Also, I believe we need to generate a diff with the upstream API to highlight the problem and see how the upstream needs to be adapted.

@geoand
Copy link
Contributor Author

geoand commented Mar 14, 2022

Okay, so what exactly would you like to see?

A first commit with all the resources (in the same or different package?) and a second commit with the current form?

Also, what code do you want in the independent project, only the resources?

@geoand
Copy link
Contributor Author

geoand commented Mar 14, 2022

Let's wait and see if the upstream client will be updated and released with the changes we need in the next few days. If so, then the copied code added here won't be needed

// where beans may or may not exist
private ClientBuilderImpl registerJacksonProviders(ClientBuilderImpl clientBuilder) {
ArcContainer arcContainer = Arc.container();
if (arcContainer == null) {
Copy link
Member

Choose a reason for hiding this comment

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

when can arc container be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially I tried to make it work it tests environments as well, but that didn't pan out. So indeed this check could be removed.

@geoand
Copy link
Contributor Author

geoand commented Mar 28, 2022

Closing in favor of #24300

@geoand geoand closed this Mar 28, 2022
@quarkus-bot quarkus-bot bot added triage/invalid This doesn't seem right and removed triage/backport? labels Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot use rest-client-reactive-jackson together with quarkus-keycloak-admin-client
5 participants