Skip to content

Conversation

@mawenge
Copy link
Contributor

@mawenge mawenge commented Nov 9, 2025

I made this change to add a parameter connection_pooling for resource_ldap_user_federation, default is true

testing:
I have made local manual test that provide this parameter and they works

image image image

@mawenge mawenge force-pushed the fix/add_connection_pooling_params branch 2 times, most recently from 72fb0bd to 13b7e01 Compare November 9, 2025 06:02
@sschu
Copy link
Contributor

sschu commented Nov 14, 2025

@mawenge Thanks for you contribution. However, there are quite some test failures. Can you have a look?

@mawenge mawenge force-pushed the fix/add_connection_pooling_params branch from 13b7e01 to 07d5fcd Compare November 15, 2025 05:18
@mawenge
Copy link
Contributor Author

mawenge commented Nov 15, 2025

@mawenge Thanks for you contribution. However, there are quite some test failures. Can you have a look?

I have fixed the tests, but after I rebase the main branch, there is a new error, and this seems not related to my change, can you please help take a look for this?

@sschu
Copy link
Contributor

sschu commented Nov 17, 2025

@mawenge The error should be fixed in main, maybe you need another rebase? Also, I assume the current default for connection pooling is disabled. We should not change the default for this value as this would be an incompatible change. Can you please let it default to false?

wenge added 5 commits November 17, 2025 20:22
@mawenge mawenge force-pushed the fix/add_connection_pooling_params branch from 563c111 to 71d7dc5 Compare November 17, 2025 12:22
@mawenge
Copy link
Contributor Author

mawenge commented Nov 17, 2025

@mawenge The error should be fixed in main, maybe you need another rebase? Also, I assume the current default for connection pooling is disabled. We should not change the default for this value as this would be an incompatible change. Can you please let it default to false?

yes, I have rebased main branch and change the default value to false, please review again, thanks

Copy link
Contributor

@sschu sschu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@sschu sschu merged commit c0e9514 into keycloak:main Nov 17, 2025
13 checks passed
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.

2 participants