Conversation
|
Thanks for submitting your first pull request! You are awesome! 🤗 |
manics
left a comment
manics
left a comment
There was a problem hiding this comment.
Thanks for opening this PR. I think a bit more discussion is needed on how to support the use-cases in #170 e.g at the moment ports aren't handled.
@bloodeagle40234 as the author of #170 do you have any comments on this PR?
|
|
||
| class LDAPAuthenticator(Authenticator): | ||
| server_address = Unicode( | ||
| server_address = List( |
There was a problem hiding this comment.
This is a breaking change, if you use traitlets.Union you should be able to maintain backwards compatibility.
| server_pool = ldap3.ServerPool(self.server_address, | ||
| ldap3.ROUND_ROBIN, | ||
| active=True, | ||
| exhaust=True) |
There was a problem hiding this comment.
With this change self.server_port is ignored
|
I already tested a couple of versions; use of ServerPool or not use; then adapted latter one on my private production environment because the server pool would not be flexible on the aspect of backward compatibility and timeout between the server switching. @manics could you suggest where contributers docs located? (mainly CLA article is minded on my company) I'll be able to prepare to push my patch with backward compatibility including unittests if you all could wait on me. |
|
I pushed my patch to my repository's branch and the diff from master is at master...bloodeagle40234:server-redirects Could you let me hear your opinion if either is fine to the upstream? My patch doesn't intend to use the ldap servers as roundrobin but has backward compatibility in the configuration and no wait penalty when switching the LDAP servers. If you prefer my version, I'll push it as a new Pull Request to the upstream repository. Thanks. |
|
What's the progress? Can we finish this please? |
6 participants
Today, we had one of our LDAP servers down, so I searched if it is possible to specify multiple servers as a list. I found this #170, which is still open. The following changes worked for me. Any thoughts?
These changes are based on https://ldap3.readthedocs.io/en/latest/server.html#server-pool.