Closed pe82 closed 6 years ago
What does this actually "fix"? That's just a default value supposed to be an example. Do you really have a production LDAP in domain "dc=my-domain,dc=com"?
Well fix for the default values or fix for sample... call it anything really -- doesn't matter. Just so other people have an easier time configuring their instance.
I have no idea why checks fail though! Must be the checks since they are just sample strings.
IIRC these defaults work OOTB when starting a local slapd with the respective layer (no idea which one exactly right now) This should be figured out and documented to lower the entry barrier when working with this package
@pe82 usually one puts in own configuration. So this string is
a) for tests as default
b) for demo purposes if someone starts the slapd
included in the buildout of this package (at least if working from the source) and uses one of its demo data sets from the ldap testing layer.
I think changing the default as you do breaks the tests, so you would have to change all the testing (kind of complex). I doubt it is worth the effort.
I'd propose to not merge this, but:
A much better method to give a hint what this field expects is providing a help text with the field here: https://github.com/collective/pas.plugins.ldap/blob/master/src/pas/plugin/ldap/properties.yaml#L53-L58 and here https://github.com/collective/pas.plugins.ldap/blob/master/src/pas/plugins/ldap/properties.yaml#L124-L129
Would you mind to add there under props
a line with help: HELP TEXT HERE
in a way you think it makes it people better understand the purpose and format of this field?
Added ou to defaults since some instances need it for successful authentication.