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

Use custom ObjectMapper for Keycloak admin client if necessary #30529

Merged
merged 3 commits into from
Jan 24, 2023

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Jan 23, 2023

Fixes: #30516

@sberyozkin
Copy link
Member

Hi @geoand It may be worth registering a custom ObjectMapper to check that this test, https://github.com/quarkusio/quarkus/blob/main/integration-tests/keycloak-authorization/src/test/java/io/quarkus/it/keycloak/AdminClientTestCase.java , is still passing

@geoand
Copy link
Contributor Author

geoand commented Jan 23, 2023

PR should be good to go now that the test was added (and another related bug fixed)

@geoand geoand marked this pull request as ready for review January 23, 2023 12:32
@sberyozkin sberyozkin self-requested a review January 23, 2023 13:20
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 Georgios, LGTM.

@geoand
Copy link
Contributor Author

geoand commented Jan 23, 2023

Looks like we need to update more of the client tests to make CI happy

@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member

sberyozkin commented Jan 23, 2023

Looks like we need to update more of the client tests to make CI happy

Sorry Georgios, looks like it is a pretty sensitive change :-)

@geoand
Copy link
Contributor Author

geoand commented Jan 23, 2023

It's a chance to do the right thing 🙂

@geoand
Copy link
Contributor Author

geoand commented Jan 23, 2023

It should be good now 🤞🏼

@sberyozkin
Copy link
Member

Getting there :-)

@geoand
Copy link
Contributor Author

geoand commented Jan 23, 2023

Darn, more failures...

I'll have a look tomorrow

@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member

We can start with the duplicate one if it proves too sensitive to optimise

@geoand
Copy link
Contributor Author

geoand commented Jan 24, 2023

This seems like the gift that keeps on giving...

@quarkus-bot

This comment has been minimized.

@geoand
Copy link
Contributor Author

geoand commented Jan 24, 2023

I believe it should be fine now.

This turned out to be a lot more than I bargained for :)

@geoand
Copy link
Contributor Author

geoand commented Jan 24, 2023

I removed the backport label because this has the potential to cause issues: Essentially we used to mix the client's and server's Jackson related MessageBodyReader / Writer classes, which now we don't (this is the proper behavior).

@quarkus-bot
Copy link

quarkus-bot bot commented Jan 24, 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.

@geoand geoand merged commit aa152df into quarkusio:main Jan 24, 2023
@quarkus-bot quarkus-bot bot added this to the 2.17 - main milestone Jan 24, 2023
@geoand geoand deleted the #30516 branch January 24, 2023 12:39
mjurc added a commit to mjurc/quarkus-test-suite that referenced this pull request Feb 3, 2023
* Adding rest-client-reactive-jackson extensions to modules using
  reactive rest client. This change is needed as the server and client
  extension has been split in quarkusio/quarkus#30529
@mjurc
Copy link

mjurc commented Feb 3, 2023

@geoand Imho this should be in https://github.com/quarkusio/quarkus/wiki/Migration-Guide-3.0#resteasy-reactive - as the decoupling can break the user apps (as it did for our tests)

@geoand
Copy link
Contributor Author

geoand commented Feb 3, 2023

I don't see why we would, because quarkus-admin-client-reactive already includes quarkus-rest-client-reactive-jackson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Customize application ObjectMapper can impact Keycloak admin client
4 participants