-
Notifications
You must be signed in to change notification settings - Fork 132
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
Replace ldap lib with ldap3 #611
base: master
Are you sure you want to change the base?
Conversation
d7fa1bb
to
2a1e630
Compare
Replace ldap with ldap3 because ldap lib is not py3 compatible. Functionality is the same.
2a1e630
to
a6e9138
Compare
python3-ldap can not be downloaded with apt-get so I just used pip install in travis script. Not sure if that satifies you. Also removed the dependency from https://github.com/SpriteLink/NIPAP/blob/master/nipap/debian/control |
Haven't read the patch yet but depending on things not available in Debian stable or the current Ubuntu (preferably including LTS) is a problem. I really want us to be as compatible with these distributions as possible and with the latest Debian stable we only depend on things that are in the standard distribution, which is nice. We could roll our own packages or come up with some elaborate scheme of supporting ldap lib on 2.7 and only using ldap3 for python3, and then we would leave it up to the user to fix the dependencies, through virtualenv or otherwise. There is a general issue with not many python libraries being available as packages in debian/ubuntu for Python3 too.. |
That's disappointing. Let's wait for official packages then. Edit: At least something is going on here: https://launchpad.net/ubuntu/+source/python-ldap3 |
Don't get me wrong here, I'm all for getting py3 compatibility and using ldap3 is a rather clear step in that direction. We just need to work out a way to have this thing supported on standard Debian & Ubuntu. This patch is fairly small and there aren't that many ldap/ldap3 calls, perhaps we can botch it to use ldap or ldap3 depending on environment. OR we just roll ldap3 as .deb and include in our debian repo. |
I just noticed that exceptions are not correctly catched when using the with statement (e.g. if ldap server is unavailable, it does not throw AuthError). Normally this shouldn't happen, so maybe it's a problem with ldap3 library. Should I switch to try/except/finally? |
I'd like to get this integrated. The one remaining blocker was how to provide an ldap3 debian package. ldap3 packages will become available in jessie which is about to be released soon. Unfortunately, I don't think we can expect everyone to upgrade very soon so instead I've decided that we'll roll our own packages (python-ldap3 and python3-ldap3 respectively) in the meantime. Users of jessie or new versions of Ubuntu will rely on their own packages while users of older distros can use our packages. There's some other work being done on our LDAP module (get AD working). That will most definitely collide with this patch but as this needs to be rebased already there is no way to avoid at least some work. Would you mind have a go at this again? :) |
@dosomder would you like to update this so that it applies cleanly on master? |
Sorry I don't have time anymore to work on this project. |
@dosomder ok, I understand. Thank you for your contributions thus far :) I will try to brush off this PR and ready it for merge. |
Replace ldap with ldap3 because ldap lib is not py3 compatible.
Functionality is the same.
#604