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

Added ldap_search module for searching in LDAP servers #126

Merged
merged 10 commits into from Apr 17, 2020
Merged

Added ldap_search module for searching in LDAP servers #126

merged 10 commits into from Apr 17, 2020

Conversation

eryx12o45
Copy link
Contributor

SUMMARY

Add a module for executing ldap_search commands on an LDAP server to query for entries which can be filtered by attributes or filterstrings.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

ldap_search

ADDITIONAL INFORMATION

The module returns a json-based string with elements that match the filter.
I've found an old version by Peter Sagerson, which was not Python3 compatible and was not working as intended. I've updated the module for Python3 and Ansible 2.10 compatibility.

tests/sanity/ignore-2.10.txt Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
@eryx12o45 eryx12o45 requested a review from gundalow April 7, 2020 10:32
plugins/modules/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/ldap_search.py Outdated Show resolved Hide resolved
@eryx12o45
Copy link
Contributor Author

@gundalow Please see the latest commit. All errors from the pipeline are already fixed now :-)

@eryx12o45 eryx12o45 requested a review from gundalow April 7, 2020 15:01
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

All new plugins should have tests. Please add unit and/or integration tests (even if the integration tests are unsupported for now).

plugins/modules/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
@ansibullbot
Copy link
Collaborator

@ansibullbot
Copy link
Collaborator

@eryx12o45 this PR contains more than one new module.

Please submit only one new module per pull request. For a detailed explanation, please read the grouped modules documentation

click here for bot help

@ansibullbot ansibullbot added backport needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor new_module New module new_plugin New plugin stale_ci CI is older than 7 days, rerun before merging labels Apr 9, 2020
@ansibullbot ansibullbot added integration tests/integration net_tools tests tests labels Apr 15, 2020
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
plugins/modules/net_tools/ldap/ldap_search.py Outdated Show resolved Hide resolved
@Andersson007
Copy link
Contributor

@gundalow @felixfontein shouldn't it be _info module?

@felixfontein
Copy link
Collaborator

@Andersson007 good question. I've been thinking about that a bit as well. _info would be formally correct, though it doesn't really convey what the module does. Maybe _query would be better, since there at least are a few modules called that way already (or at least were in 2.9; there was none called _search), and ldap_query would still be expressive enough for users to understand what it does. (From a module called ldap_info I would expect it retrieves general information about the LDAP server, like number of entries, server version, etc.)

@Andersson007
Copy link
Contributor

@felixfontein maybe ldap_search_info?
the word query can assume it can be able to modify something (in general)

@felixfontein
Copy link
Collaborator

_search_info sounds strange, as if the module would return information about a search (instead of doing a search). And at least the English word query does not imply changes (https://www.merriam-webster.com/dictionary/query).

@Andersson007
Copy link
Contributor

Andersson007 commented Apr 16, 2020

@felixfontein practicaly it can imply modifications https://www.postgresql.org/docs/current/sql-update.html for example

@felixfontein
Copy link
Collaborator

I would argue that anyone who is familiar with LDAP wouldn't assume this :) It would be really nice to have more opinions and ideas though...

@Andersson007
Copy link
Contributor

Andersson007 commented Apr 16, 2020

@felixfontein search or query sound equal to me in terms of proper module naming:)
Interesting point is that should it be _info module if it returns only information?
If it's ok as it is now, it can be _search or _query:)

@Andersson007
Copy link
Contributor

Andersson007 commented Apr 16, 2020

sorry, i didn't want to close the pr:) the idea LGTM:)

@felixfontein
Copy link
Collaborator

I don't think _info doesn't make that much sense for modules searching in databases (for most other cases, it is). This whole thing would also make sense as a lookup plugin, but (and a very important but) these can only be run on the controller, not on the remote nodes. Which for example would make it impossible for me to use this if it were a lookup plugin.

@Andersson007
Copy link
Contributor

@felixfontein ok, so ldap_search for me personally sounds good)

@felixfontein
Copy link
Collaborator

Ok, so if nobody else has an opinion, let's keep ldap_search :)

@eryx12o45
Copy link
Contributor Author

@felixfontein @Andersson007 Thanks a lot four your support here. I've updated the code with your suggestions. Hopefully the way you've meant them ^^

Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you guys
shipit

@felixfontein felixfontein merged commit e3e6c61 into ansible-collections:master Apr 17, 2020
@felixfontein
Copy link
Collaborator

@eryx12o45 thanks a lot for reviving and updating this module, and especially for adding tests! :)

@eryx12o45
Copy link
Contributor Author

@felixfontein @gundalow @Andersson007 Thanks a lot. Would it be possible to get a new release 0.1.2 of the collection, because otherwise unfortunately I'm not able to install it via ansible-galaxy :-/

@felixfontein
Copy link
Collaborator

@eryx12o45 it definitely won't be 0.1.2, since according to semantic versioning patch releases cannot have new features :) I'm not sure when a new release will happen; the discussion how release manangement etc. should go are still going on.

@eryx12o45
Copy link
Contributor Author

@felixfontein I see, sorry didn't had that insight before, but this absolutely makes sense.

@felixfontein
Copy link
Collaborator

@eryx12o45 no problem, the current versioning docs are not really there / somewhat hidden, and the whole discussion is also happening pretty hidden... there are a lot more people wondering on what's happening ;)

@felixfontein
Copy link
Collaborator

@eryx12o45 ansible/ansible#69154 will allow to install collections from git repositories

@eryx12o45
Copy link
Contributor Author

@felixfontein Great! Thanks a lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration tests/integration module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR new_contributor Help guide this first time contributor new_module New module new_plugin New plugin tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants