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

Canonicalize and ensure uniqueness of indices #28

Merged
merged 1 commit into from
Jul 21, 2016

Conversation

tcsalameh
Copy link
Contributor

Prior to this commit, adding a entryUUID or entryCSN
index conflicted with those being added by default
when the syncprov overlay is enabled.

This avoids that behavior by canonicalizing and uniquing
the indices before adding them into OpenLDAP.

@bodgit
Copy link
Owner

bodgit commented Jul 20, 2016

I'm trying to understand this change.

  1. Do you have a test case or example for this? What other indices do you need for entryUUID/entryCSN?
  2. Your new function appears to be something supported by Puppet 4 only. I don't want to drop Puppet 3 support; writing this in terms of a parser function would work on both.

There will also need to be tests written for this.

@tcsalameh
Copy link
Contributor Author

Hi Matt,

Thanks for responding so quickly - I'm happy to clarify. I've cc'd Jesse
Hathaway, who I'm working with.

  1. The idea is to make the behavior more lenient in the case where someone
    specifies indices that are already included when syncprov is set to true.
    The example would be a user who sets syncprov => true and also adds indices
    for entryUUID and entryCSN up front. These indices are added again in
    config.pp, which causes LDAP to reject the duplicate indices and the puppet
    apply to fail. It seems more user-friendly to just reformat and unique the
    index entries before adding them to LDAP, rather than completely fail in
    the case that someone accidentally adds duplicate indices.
  2. Good point - I've rewritten it as a parser function.

We had some trouble getting the tests to run yesterday, but got them
working today. I added several test cases for the new function, and had to
modify a few existing test cases to account for the different format for
the olcDbIndex attributes. I've amended the pull request to include these
changes.

Best,
Tom Christiansen-Salameh

On Wed, Jul 20, 2016 at 4:49 PM, Matt Dainty notifications@github.com
wrote:

I'm trying to understand this change.

  1. Do you have a test case or example for this? What other indices do
    you need for entryUUID/entryCSN?
  2. Your new function appears to be something supported by Puppet 4
    only. I don't want to drop Puppet 3 support; writing this in terms of a
    parser function would work on both.

There will also need to be tests written for this.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#28 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIXiUN-V1gUa0UfGWWI_w-qHKsVdkqm7ks5qXpfigaJpZM4JQ3Sm
.

@coveralls
Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage increased (+0.7%) to 53.527% when pulling 9a7bddd on tcsalameh:unique-indices into 9a14f51 on bodgit:master.

Prior to this commit, adding a entryUUID or entryCSN
index conflicted with those being added by default
when the syncprov overlay is enabled.

This avoids that behavior by canonicalizing and uniquing
the indices before adding them into OpenLDAP.
@coveralls
Copy link

coveralls commented Jul 21, 2016

Coverage Status

Coverage increased (+0.7%) to 53.527% when pulling 91f7c71 on tcsalameh:unique-indices into 9a14f51 on bodgit:master.

@bodgit bodgit added the bug label Jul 21, 2016
@bodgit bodgit added this to the v1.1.4 milestone Jul 21, 2016
@bodgit bodgit self-assigned this Jul 21, 2016
@bodgit bodgit merged commit 5d39d36 into bodgit:master Jul 21, 2016
@bodgit
Copy link
Owner

bodgit commented Jul 21, 2016

Ok, the change looks good to me. Tests all pass. 👍

Thanks for your contribution!

@tcsalameh
Copy link
Contributor Author

Thank you! We're using this module as part of an OpenLDAP implementation project, so I may be submitting a couple more pull requests in the next few weeks.

@tcsalameh tcsalameh deleted the unique-indices branch July 21, 2016 21:59
@bodgit
Copy link
Owner

bodgit commented Jul 21, 2016

Thank you! We're using this module as part of an OpenLDAP implementation project, so I may be submitting a couple more pull requests in the next few weeks.

No problem. I've pushed v1.1.4 to the forge.

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

Successfully merging this pull request may close these issues.

None yet

3 participants