-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 Client config builder #44697
Add OIDC Client config builder #44697
Conversation
This comment has been minimized.
This comment has been minimized.
🙈 The PR is closed and the preview is expired. |
This comment has been minimized.
This comment has been minimized.
a2a407d
to
8023081
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I mentioned I made mistake in one test so it didn't test what I wanted. I'll push in a minute. UPDATE: done |
8023081
to
d66c643
Compare
This comment has been minimized.
This comment has been minimized.
Hi @michalvavrik, thanks, as far as the builder is concerned, did you follow the same pattern you did for the OIDC dynamic client config, where the whole OIDC client can be built with a single sequence, with sub-config-mappings having their own builders, which can be used independently and then plugged into into the main sequence or used inside that sequence with |
...nsions/oidc-client/runtime/src/main/java/io/quarkus/oidc/client/OidcClientConfigBuilder.java
Outdated
Show resolved
Hide resolved
Looks like it is the case, sound good |
Yes, I believe so. And I added many shortcuts, but these things are quite opinionated, please let me know if you want another methods added to the builder. Thanks |
@michalvavrik As far as shortcuts are concerned, we can add them more if we notice users have to type some sub-optimal code, the important thing is that they have options how to structure the complete build sequence, without having a single flat builder to set everything |
I am just working on it. Will push it soon. |
d66c643
to
5a51c3a
Compare
Status for workflow
|
Status for workflow
|
btw I have checked https://quarkus-pr-main-44697-preview.surge.sh/version/main/guides/all-config with "quarkus.oidc-client." and it looks fine (javadoc there). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @michalvavrik, all is ready for the last one :-), thanks very much
that's already WIP, looking forward to finishing it in a day or two. until then! |
@ConfigMapping
#39185oidcClients.newClient(formerConfigClass)
while we migrated to the config mapping; that should reduce copying between config mapping interfaces and config classesid
is set if user didn't specify it explicitly. In past, we set this config class property inside of the recorder, but now it needs to be set bit sooner; situation shouldn't change for users though