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

Replace ldap lib with ldap3 #611

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dosomder
Copy link
Contributor

@dosomder dosomder commented Sep 9, 2014

Replace ldap with ldap3 because ldap lib is not py3 compatible.
Functionality is the same.

#604

@dosomder dosomder force-pushed the backend/604-py3-ldap3 branch 2 times, most recently from d7fa1bb to 2a1e630 Compare September 9, 2014 12:28
Replace ldap with ldap3 because ldap lib is not py3 compatible.
Functionality is the same.
@dosomder
Copy link
Contributor Author

dosomder commented Sep 9, 2014

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

@plajjan
Copy link
Member

plajjan commented Sep 9, 2014

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..

@dosomder
Copy link
Contributor Author

dosomder commented Sep 9, 2014

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

@plajjan
Copy link
Member

plajjan commented Sep 10, 2014

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.

@dosomder
Copy link
Contributor Author

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?

@plajjan
Copy link
Member

plajjan commented Feb 2, 2015

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? :)

@plajjan
Copy link
Member

plajjan commented Apr 13, 2015

@dosomder would you like to update this so that it applies cleanly on master?

@dosomder
Copy link
Contributor Author

Sorry I don't have time anymore to work on this project.

@plajjan
Copy link
Member

plajjan commented Apr 15, 2015

@dosomder ok, I understand. Thank you for your contributions thus far :) I will try to brush off this PR and ready it for merge.

@plajjan plajjan self-assigned this Apr 16, 2015
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