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

Add python-ldap module to conditional requirements #2576

Merged
merged 3 commits into from Jul 27, 2016

Conversation

Projects
None yet
6 participants
@abretaud
Copy link
Contributor

commented Jun 29, 2016

When using the ldap auth method, the python-ldap module is required

@abretaud abretaud referenced this pull request Jun 29, 2016

Merged

Add python-ldap #97

@galaxybot galaxybot added the triage label Jun 29, 2016

@galaxybot galaxybot added this to the 16.07 milestone Jun 29, 2016

@@ -76,6 +89,9 @@ def check_pygments( self ):
# pygments is a dependency of weberror and only weberror
return self.check_weberror()

def check_python_ldap( self ):
return 'ldap' in self.authenticators

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jun 29, 2016

Member

or 'activedirectory' in self.authenticators

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Jun 29, 2016

Thanks @abretaud, that's sweet!

@abretaud

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2016

Somewhat related thing: do you know if we can have access to the auth method(s) from templates? The "Forgot password?" link at https://github.com/galaxyproject/galaxy/blob/dev/templates/user/login.mako#L91 sometimes confuses our users as it as no effect with ldap auth. It would be nice to hide it in this case

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Jun 29, 2016

@abretaud It may be technically possible, but I don't think that's a good idea because you may have multiple authentication available at the same time (e.g. LDAP for internal users and Galaxy password database for external ones).

@abretaud

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2016

Oh yes, I see the point. Maybe it could be hidden only for simple cases (e.g. when there is only ldap auth configured)? Anyway, it's just a cosmetic detail, so no worry

@erasche

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

@nsoranzo is this ready to merge?

@bgruening

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

I have hope this will fix this one too: bgruening/docker-galaxy-stable#199

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

@erasche I think we have to wait for galaxyproject/starforge#97 to be merged and for the wheels to be online.

@erasche

This comment has been minimized.

Copy link
Member

commented Jun 30, 2016

@nsoranzo ok, thanks, just following up on a low hanging fruit. :)

@dannon dannon merged commit 28e7f86 into galaxyproject:dev Jul 27, 2016

4 checks passed

api test Build finished. 224 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 111 tests run, 0 skipped, 0 failed.
Details
toolshed test Build finished. 582 tests run, 0 skipped, 0 failed.
Details
@dannon

This comment has been minimized.

Copy link
Member

commented Jul 27, 2016

Merged this to get it in, we can follow up on the wheel on the starforge side. This just enables the conditional dependency, which will force this to be built/installed from pypi until we get the wheel up, which is not a bad thing.

@@ -10,3 +10,4 @@ pbs_python
drmaa
statsd
# PyRods not in PyPI
python-ldap==2.4.25

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Jul 27, 2016

Member

A 2.4.26 release came out in the mean time, if you want to update here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.