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

Support setting maxPoolSize for OIDC WebClients #17701

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

glefloch
Copy link
Member

@glefloch glefloch commented Jun 4, 2021

This allows a user setting the maxPoolSize of the Web client.

close #17666

@glefloch glefloch requested a review from sberyozkin June 4, 2021 16:40
Copy link
Member

@machi1990 machi1990 left a comment

Choose a reason for hiding this comment

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

LGTM

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 4, 2021

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

Failing Jobs - Building af7dfb1

Status Name Step Test failures Logs Raw logs
JVM Tests - JDK 11 Build Test failures Logs Raw logs
JVM Tests - JDK 16 Build Test failures Logs Raw logs
Native Tests - Security2 Build Test failures Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/oidc/deployment

io.quarkus.oidc.test.CodeFlowDevModeTestCase.testAccessAndRefreshTokenInjectionDevMode line 43 - More details - Source on GitHub


⚙️ JVM Tests - JDK 16 #

📦 extensions/oidc/deployment

io.quarkus.oidc.test.CodeFlowDevModeTestCase.testAccessAndRefreshTokenInjectionDevMode line 43 - More details - Source on GitHub


⚙️ Native Tests - Security2 #

📦 integration-tests/oidc-tenancy

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testResolveTenantIdentifierWebAppDynamic - More details - Source on GitHub

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testSimpleOidcNoDiscovery - More details - Source on GitHub

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testSimpleOidcJwtWithJwkRefresh - More details - Source on GitHub

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testOpaqueTokenIntrospectionDisallowed - More details - Source on GitHub

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testJwtTokenIntrospectionDisallowed - More details - Source on GitHub

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testResolveTenantConfig - More details - Source on GitHub

@sberyozkin
Copy link
Member

sberyozkin commented Jun 4, 2021

Hmm, strange test errors, the PR does not seem to be related

@sberyozkin
Copy link
Member

May be it is a rebase problem ? The other PRs seem to be OK

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 5, 2021

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

Failing Jobs - Building 0f85d03

Status Name Step Test failures Logs Raw logs
JVM Tests - JDK 11 Build Test failures Logs Raw logs
JVM Tests - JDK 16 Build Test failures Logs Raw logs
Native Tests - Security2 Build Test failures Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/oidc/deployment

io.quarkus.oidc.test.CodeFlowDevModeTestCase.testAccessAndRefreshTokenInjectionDevMode line 43 - More details - Source on GitHub


⚙️ JVM Tests - JDK 16 #

📦 extensions/oidc/deployment

io.quarkus.oidc.test.CodeFlowDevModeTestCase.testAccessAndRefreshTokenInjectionDevMode line 43 - More details - Source on GitHub


⚙️ Native Tests - Security2 #

📦 integration-tests/oidc-tenancy

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testResolveTenantIdentifierWebAppDynamic - More details - Source on GitHub

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testSimpleOidcNoDiscovery - More details - Source on GitHub

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testSimpleOidcJwtWithJwkRefresh - More details - Source on GitHub

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testOpaqueTokenIntrospectionDisallowed - More details - Source on GitHub

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testJwtTokenIntrospectionDisallowed - More details - Source on GitHub

io.quarkus.it.keycloak.BearerTokenAuthorizationInGraalITCase.testResolveTenantConfig - More details - Source on GitHub

@glefloch
Copy link
Member Author

glefloch commented Jun 5, 2021

@sberyozkin I addressed your comments and rebase, but I still have those weird failures..

@sberyozkin
Copy link
Member

@glefloch Good morning :-), this is strange as I can see other PRs being green. Do these tests pass locally in your branch ?

@sberyozkin
Copy link
Member

@glefloch Hi Guillaume, sorry it is proving a bit more difficult than it should've been :-). But indeed since we have the users sometimes creating OidcTenantConfig manually then all the fields should be initialized. I did not realize it yesterday

@glefloch
Copy link
Member Author

glefloch commented Jun 5, 2021

@sberyozkin thanks for those tips, I updated the branch and tests pass locally.

@sberyozkin sberyozkin self-requested a review June 5, 2021 16:10
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.

@glefloch thanks for your help with fixing this issue :-)

@sberyozkin sberyozkin added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 5, 2021
gsmet
gsmet previously requested changes Jun 7, 2021
* The maximum size of the connection pool used by the WebClient
*/
@ConfigItem
public OptionalInt webClientMaxPoolSize = OptionalInt.empty();
Copy link
Member

Choose a reason for hiding this comment

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

Isn't connectionTimeout also related to the web client? If so, being consistent and dropping the webClient prefix would make sense.

Let's decide this before merging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes this is related, it is used in the same method to set WebClientOption

Copy link
Member

Choose a reason for hiding this comment

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

@gsmet, @glefloch sure, maxPoolSize works as well, thanks

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the property name, and all related reference (getter/setter, ...)

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 8, 2021

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

Failing Jobs - Building 31281db

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 8, 2021

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

Failing Jobs - Building 31281db

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@sberyozkin sberyozkin dismissed gsmet’s stale review June 8, 2021 12:29

webClient qualifier has been dropped to make it consistent with the other properties affecting WebClientOptions

@sberyozkin sberyozkin merged commit 8f21a26 into quarkusio:main Jun 8, 2021
@quarkus-bot quarkus-bot bot added this to the 2.1 - main milestone Jun 8, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 8, 2021
@glefloch glefloch deleted the fix/17666 branch August 19, 2021 06:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support configuring Vert.x WebClient maxPoolSize in quarkus-oidc and quarkus-oidc-client
4 participants