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

Fixes #45417 #45778

Merged
merged 2 commits into from
Sep 25, 2018
Merged

Fixes #45417 #45778

merged 2 commits into from
Sep 25, 2018

Conversation

gustavomcarmo
Copy link
Contributor

SUMMARY

Fixes #45417

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

ldap_entry

ANSIBLE VERSION
2.7

@ansibot
Copy link
Contributor

ansibot commented Sep 18, 2018

cc @jtyr
click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. net_tools Net-tools category small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Sep 18, 2018
@ryansb ryansb removed the needs_triage Needs a first human triage before being processed. label Sep 18, 2018
@@ -145,7 +145,7 @@ def _load_attrs(self):
attrs[name] = []

if isinstance(value, list):
attrs[name] = [map(to_bytes, value)]
attrs[name] = [to_bytes(v) for v in value]
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be the same thing:

In [1]: s = "abcdef"

In [2]: [map(str.upper, s)]
Out[2]: [['A', 'B', 'C', 'D', 'E', 'F']]

In [3]: [str.upper(v) for v in s]
Out[3]: ['A', 'B', 'C', 'D', 'E', 'F']

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure this is not the same thing. The first does not solve the issue.

Copy link
Contributor

@jtyr jtyr Sep 19, 2018

Choose a reason for hiding this comment

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

Right. I can see now what's happening. You previous fix changed this:

-attrs[name] = value
+attrs[name] = [map(to_bytes, value)]

Which is wrong as it creates list of list. So this PR is fixing that which is good but I would rather use this syntax than the for loop:

attrs[name] = map(to_bytes, value)

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 also prefer map instead of a loop, but when map(to_bytes, value) is used, as you've asked for, the following error happens:

The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_3kcj1hb2/ansible_module_ldap_entry.py", line 241, in main
    action()
  File "/tmp/ansible_3kcj1hb2/ansible_module_ldap_entry.py", line 157, in _add
    self.connection.add_s(self.dn, modlist)
  File "/home/gcarmo/.local/lib/python3.6/site-packages/ldap/ldapobject.py", line 428, in add_s
    return self.add_ext_s(dn,modlist,None,None)
  File "/home/gcarmo/.local/lib/python3.6/site-packages/ldap/ldapobject.py", line 413, in add_ext_s
    msgid = self.add_ext(dn,modlist,serverctrls,clientctrls)
  File "/home/gcarmo/.local/lib/python3.6/site-packages/ldap/ldapobject.py", line 410, in add_ext
    return self._ldap_call(self._l.add_ext,dn,modlist,RequestControlTuples(serverctrls),RequestControlTuples(clientctrls))
  File "/home/gcarmo/.local/lib/python3.6/site-packages/ldap/ldapobject.py", line 329, in _ldap_call
    reraise(exc_type, exc_value, exc_traceback)
  File "/home/gcarmo/.local/lib/python3.6/site-packages/ldap/compat.py", line 44, in reraise
    raise exc_value
  File "/home/gcarmo/.local/lib/python3.6/site-packages/ldap/ldapobject.py", line 313, in _ldap_call
    result = func(*args,**kwargs)
ldap.PARAM_ERROR: {'desc': 'Bad parameter to an ldap routine', 'matched': 'ou=people,dc=example,dc=org'}

We can investigate it or just assume the solution is ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

Try this:

attrs[name] = list(map(to_bytes, value))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could @Akasurde do it?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can add it into this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which case the ldap_attr module goes wrong after the @Akasurde's PR #41248 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtyr I've just found out how the ldap_attr module goes wrong after the @Akasurde's PR #41248. When adding a value to an attribute, the exception TypeError: object of type 'filter' has no len() is thrown, on line 183. The solution is values_to_add = list(filter(self._is_value_absent, self.values)) on line 181. I have not tested the value deletion, the same error may occur. I'll make a new PR soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtyr @Akasurde @webknjaz I've just created the PR #46818 to fix the bug mentioned above.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Sep 19, 2018
@gustavomcarmo
Copy link
Contributor Author

@Akasurde could you please review this PR and merge it?

@webknjaz webknjaz merged commit 7a74734 into ansible:devel Sep 25, 2018
@gustavomcarmo gustavomcarmo deleted the 45417_fix branch September 25, 2018 10:28
Akasurde pushed a commit to Akasurde/ansible that referenced this pull request Oct 14, 2018
abadger pushed a commit that referenced this pull request Oct 15, 2018
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. net_tools Net-tools category small_patch support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to add ldap entry
5 participants