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

ldap_attrs: search_s based _is_value_present #5385

Merged
merged 10 commits into from
Oct 25, 2022

Conversation

mrvanes
Copy link
Contributor

@mrvanes mrvanes commented Oct 19, 2022

SUMMARY

Fixes #977

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

community.general.ldap_attrs

ADDITIONAL INFORMATION

Instead of comparing the dn's attribute values using compare_s, I suggest using search_s plus filter, because search is X-ORDERED agnostic.

@ansibullbot
Copy link
Collaborator

@ansibullbot ansibullbot added bug This issue/PR relates to a bug module module needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR net_tools new_contributor Help guide this first time contributor plugins plugin (any type) labels Oct 19, 2022
@ansibullbot

This comment was marked as outdated.

@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI has_issue labels Oct 19, 2022
@ansibullbot

This comment was marked as outdated.

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.

Thanks for your contribution! Please add a changelog fragment. Thanks.

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-4 labels Oct 19, 2022
@mrvanes
Copy link
Contributor Author

mrvanes commented Oct 19, 2022

I wonder why the tests fail on importing ldap in ldap_attrs.py? What can be done?
edit: never mind, I see the superfluous import

@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Oct 19, 2022
@mrvanes
Copy link
Contributor Author

mrvanes commented Oct 19, 2022

Before merging: I see that ldap_entry.py only catches ldap.NO_SUCH_OBJECT on search_s and so raises ldap.INVALID_DN_SYNTAX on malformed dn. Do we want to break on malformed dn's so that the user is informed of bad config or silently ignore and return _is_present = False like I implemented?

@mrvanes
Copy link
Contributor Author

mrvanes commented Oct 21, 2022

CI is happy and there's nothing I want to change. How to proceed from here?

@felixfontein
Copy link
Collaborator

@mrvanes basically this is waiting for a review by the module maintainers.

@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Oct 21, 2022
@felixfontein felixfontein changed the title search_s based _is_value_present ldap_attrs: search_s based _is_value_present Oct 21, 2022
This was referenced Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue/PR relates to a bug has_issue module module net_tools new_contributor Help guide this first time contributor plugins plugin (any type)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ldap_attrs: 'Type or value exists' is fatal when it should be OK
4 participants