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

Remove manual LDAP search pagination on UGM principal search call… #53

Merged
merged 3 commits into from
Dec 15, 2017

Conversation

rnixx
Copy link
Contributor

@rnixx rnixx commented Dec 14, 2017

…s. This is done in downstream API as of node.ext.ldap 1.0b7.

Relates to conestack/node.ext.ldap#36

@jensens @pbauer @fredvd @mauritsvanrees May you be so kind to check the changes?

…s. This is done in downstream API as of ``node.ext.ldap`` 1.0b7.
Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

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

LGTM.
Of course, this depends on version 1.0b7 being available, which is not yet the case. I don't know if that is why Travis fails, could be.
1.0b7 would need to be added to the install_requires in setup.py as minimum version though.

TODO.rst Outdated
[ ] - group in group (depends on: node.ext.ldap: group.groups support)
[ ] - roles from ldap
[ ] - Option on LDAP inspector whether to use query filters from users and
groups config
Copy link
Member

Choose a reason for hiding this comment

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

This is MarkDown syntax, which PyPI does not support. In this case, for the last line it gives '(ERROR/3) Unexpected indentation.' Also, for MarkDown it should be the other way around: - [ ].

You can use restview TODO.rst to check the ReStructuredText parsing of this file. Or restview --long-description to check the long description from setup.py that would end up on PyPI.
See https://pypi.org/project/restview/

Copy link
Member

Choose a reason for hiding this comment

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

The longdescription command from zest.releaser works too, and is easier to type, but I prefer restview these days.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just finish the work on node.ext.ldap 1.0b7, will fix with the next commit defining the node.ext.ldap dependency version

@@ -8,7 +8,8 @@ Setup
Basics
------

::
.. code-block:: pycon
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't pycon be python? Hm, it works though when I try it with restview, and when I use a random string instead it gives an error. So pycon is apparently okay.
I see very minor differences. Is pycon maybe meant as variant that shows better in presentations, for example on the PyCon conference? Do you know that? Just curious. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pycon is python console

Copy link
Member

Choose a reason for hiding this comment

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

Ah! Thanks for the info.

@rnixx
Copy link
Contributor Author

rnixx commented Dec 15, 2017

node.ext.ldap 1.0b7 is released. @mauritsvanrees from my POV we can merge and release.
Should I? Or you? Anyone?

@jensens jensens merged commit b561c38 into master Dec 15, 2017
@jensens jensens deleted the rnixx_ugm_principals_search_signature_fix branch December 15, 2017 12:42
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.

3 participants