add support for require ldap-group #232
Conversation
|
Hello @wyardley ! I am interested for help , thanks |
|
Hi -- #voxpupuli on Freenode IRC or #voxpupuli / #testing on the Puppet slack are good places to get help with tests. Normally, when reworking something like this, it's ideal to preserve attribution to the original author in the commit (ideally by cherry-picking their commit), but I don't know that it's required. |
|
hello @wyardley , Thanks, |
There was a problem hiding this comment.
This looks about right, see comments on tests, though. Ideally, leave the existing context, and add a new one, testing this specifically.
Also, your GitHub username doesn't seem to be tied to the address you're using for commits.
https://help.github.com/articles/setting-your-commit-email-address-in-git/
Can you squash the commits in your branch?
https://github.com/todotxt/todo.txt-android/wiki/Squash-All-Commits-Related-to-a-Single-Issue-into-a-Single-Commit
| @@ -126,6 +126,8 @@ class { 'puppetboard::apache::conf': | |||
| ldap_bind_dn => 'cn=user,dc=puppet,dc=example,dc=com', | |||
| ldap_bind_password => 'password', | |||
| ldap_url => 'ldap://puppet.example.com', | |||
| ldap_require_group => true, | |||
There was a problem hiding this comment.
Hi - Yes, I think this is pretty close. But we should test both cases, so one context for ldap auth, and test that the config file does not contain "Require ldap-group".
Then make another conext that sets those parameters explicitly, and test for the expected results.
manifests/apache/vhost.pp
Outdated
| @@ -88,6 +96,8 @@ | |||
| Optional[String] $ldap_bind_password = undef, | |||
| Optional[String] $ldap_url = undef, | |||
| Optional[String] $ldap_bind_authoritative = undef, | |||
| Boolean $ldap_require_group = $::puppetboard::params::ldap_require_group, | |||
There was a problem hiding this comment.
I believe the leading colons are not needed here anymore, so
$puppetboard::params...
manifests/apache/conf.pp
Outdated
| @@ -69,7 +77,9 @@ | |||
| Optional[String] $ldap_bind_dn = undef, | |||
| Optional[String] $ldap_bind_password = undef, | |||
| Optional[String] $ldap_url = undef, | |||
| Optional[String] $ldap_bind_authoritative = undef | |||
| Optional[String] $ldap_bind_authoritative = undef, | |||
| Boolean $ldap_require_group = $::puppetboard::params::ldap_require_group, | |||
There was a problem hiding this comment.
$puppetboard:: vs $::puppetboard.
|
I'd also acknowledge @rendhalver in the commit message if you can't get the commit directly attributed to him in your branch. |
|
New PR #236 |
Pull Request (PR) description
add support for require ldap-group