-
-
Notifications
You must be signed in to change notification settings - Fork 272
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
openldap - add binddn and password parameters #657
Conversation
|
Hi @leonkyneur, thanks for the PR. are you able to add spec tests as well? |
|
Hi @bastelfreak may need guidance on the tests - I did look at the existing spec tests and given these params are just part of the instances hash and are optional - I couldn't see merit extending the existing tests as there is no validation on the values of 'instances'. Do you think by extending the current tests to just include these params and checking with "contain_file().with(content:" suffice? Or would you prefer some way of iterating the hash and checking the parameters? |
|
@leonkyneur it will be enough to just extend the params hash and checking the generated file, yes. I see the ldap spec isn't very easy to read, but that shouldn't be too difficult to modify. If you feel up to it, a re-format of the spec using eg. heredoc syntax might make it easier to extend. |
|
agree with you @oranenj re heredoc - I've modified tests to utilize this. Latest commit includes the new params in the spec test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leonkyneur good stuff. Would you mind squashing the rubocop fix with the test reformatting commit?
I'm approving this either way. I also updated the title of the pull request a bit, since it will show up in the changelog
863138c
to
26ad0f6
Compare
|
@oranenj commit squashed. Thanks |
|
And merged. Thanks! |
These parameters are required if the directory server requires authentication to retrieve statistics.