Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

Adding ldap_entry module #2952

Closed
wants to merge 3 commits into from
Closed

Adding ldap_entry module #2952

wants to merge 3 commits into from

Conversation

jtyr
Copy link
Contributor

@jtyr jtyr commented Sep 18, 2016

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

ldap_entry

ANSIBLE VERSION

For Ansible 2.3

SUMMARY

This PR is adding a new module that allows to add/change/remove LDAP entries. The module is based on the code from Peter Sagerson who gave me the permission to publish it.

@jtyr jtyr force-pushed the jtyr-ldap_entry branch 3 times, most recently from 0a11eb6 to c54b7e6 Compare September 18, 2016 00:31
@gregdek
Copy link
Contributor

gregdek commented Sep 18, 2016

Thanks @jtyr for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

@jangrewe
Copy link

jangrewe commented Nov 3, 2016

shipit

Works great for me (compared to the original one, which doesn't)

@drybjed
Copy link
Contributor

drybjed commented Nov 7, 2016

I have tested this module with a few Ansible roles with different options, it seems to work as expected, grat job.

shipit

requirements:
- python-ldap
options:
'...':
Copy link
Member

Choose a reason for hiding this comment

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

we normally document this as _raw_params

Copy link
Member

Choose a reason for hiding this comment

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

I would actually prefer an 'entry/object' which is a freeform dict

description:
- Option used to allow the user to overwrite any of the other
options. To remove an option, set the value of the option to
C(null).
Copy link
Member

Choose a reason for hiding this comment

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

this description is very confusing

- simpleSecurityObject
- organizationalRole
description: An LDAP administrator
userPassword: "{SSHA}tabyipcHzhwESzRaGA7oQ/SDoBZQOGND"
Copy link
Member

Choose a reason for hiding this comment

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

see my point above, this seems confusing as the 'non existing options' are slurped into the entry definitions, there is no clear separation between the ldap entry properties and the task options. A simple misspelling will also create havok on entries.

prams: hello=kitty

@gregdek
Copy link
Contributor

gregdek commented Nov 8, 2016

@jtyr A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer.

[This message brought to you by your friendly Ansibull-bot.]

@jtyr
Copy link
Contributor Author

jtyr commented Nov 13, 2016

@bcoca I have fixed the things you commented on. Please feel free to review it again.

@drybjed Please could you run some more tests with the current implementation?

@drybjed
Copy link
Contributor

drybjed commented Nov 13, 2016

@jtyr Nice work with the attributes parameter. I assume that existing roles that used the free form attribute specification will need to be fixed?

@jtyr
Copy link
Contributor Author

jtyr commented Nov 13, 2016

@drybjed Yes, the tasks using the old version of the module must be modified. Fortunately it's quite simple - just add the attributes option and increase indentation of all non-module options to fall under the attributes option.

@drybjed
Copy link
Contributor

drybjed commented Nov 15, 2016

@jtyr I have checked the new ldap_entry changes you added, unfortunately the module fails. The play I've used is:

- hosts: ldap
  become: True

  tasks:
    - name: Create main entry
      ldap_entry:
        dn: 'ou=test,dc=example,dc=com'
        objectClass: 'organizationalUnit'

The task fails with:

fatal: [ldap]: FAILED! => {"changed": false, "failed": true, "msg": "Some entry attributes must be defined."}

Any idea what attributes would be needed? I checked this on the slapd server and for organizationalUnit, top, nothing important seems to be required.

NB, this is also one of the examples in the module documentation, more or less.

@jtyr
Copy link
Contributor Author

jtyr commented Nov 15, 2016

Thanks for running the tests, @drybjed. I have removed the requirement for having some attributes defined for every entry. Your example should work now.

@drybjed
Copy link
Contributor

drybjed commented Nov 15, 2016

@jtyr Great, looks like it works now, after the necessary changes to tasks are applied. I'm going to switch my copy of ldap_entry to this version.

shipit

@jtyr
Copy link
Contributor Author

jtyr commented Nov 15, 2016

Thanks for testing, @drybjed.

@bcoca Please could you review it now?

@gregdek
Copy link
Contributor

gregdek commented Nov 23, 2016

@jtyr Another friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open. If you have addressed the issues and believe it's ready for review, please comment with the text "ready_for_review". If we don't hear from you within another 14 days, we will close this pull request.

[This message brought to you by your friendly Ansibull-bot.]

@ansibot
Copy link

ansibot commented Dec 7, 2016

This repository has been locked. All new issues and pull requests should be filed in https://github.com/ansible/ansible

Please read through the repomerge page in the dev guide. The guide contains links to tools which automatically move your issue or pull request to the ansible/ansible repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants