Privileged ldap#157
Conversation
dkess
left a comment
There was a problem hiding this comment.
Thanks for doing this, it's a welcome change. I think it should be slightly re-organized but other than that this lgtm!
|
Code looks good, however now tests are failing because the test runner can't find the password file. I think the best way to deal with this would be to move the tests to the |
|
I'm going to try and mock the function instead of moving the tests to manual (where they may never get run), but that isn't working yet |
|
Thanks for this, I think the new code is good now. Instead of mocking functions, I'd say it's fine to give the test runner access to the password (it's accessible to all staff anyways) and actually running the functions. This will probably require a PR to Puppet. |
dkess
left a comment
There was a problem hiding this comment.
lgtm, however at some point we should remove the mock and really test the function
|
This commit needs to be temporary reverted, as it breaks ocfweb. We got the following rootspam this morning: (i've redacted some extra information here) Looks like this is happening because the password file isn't inside the docker container that ocfweb is running in. We'll have to change the container configuration. The password should also perhaps be part of the ocfweb development configuration. |
addresses #142