-
Notifications
You must be signed in to change notification settings - Fork 82
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
Added ensure_resource and ensure_packages to avoid collisions #26
Conversation
@@ -1,20 +1,18 @@ | |||
# See README.md for usage information | |||
class sssd::install ( | |||
$ensure = $sssd::ensure, |
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.
I don't see why this change is necessary. The old variable name seems fine.
I would't want to break anyone's configuration, that is currently using this class directly...
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.
Just realised my error here - think I merged more than I should have from the earlier PR. Also probably explains why the variable wasn't picked up above.
I'll clean that up tomorrow.
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.
All cleaned up now.
@@ -123,7 +123,9 @@ | |||
validate_re($service_ensure, '^running|true|stopped|false$') | |||
|
|||
anchor { 'sssd::begin': } -> | |||
class { '::sssd::install': } -> | |||
class { '::sssd::install': | |||
ensure => $sssd_package_ensure, |
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.
I think that reading this var from the install class is cleaner...
Can we leave this code alone? Is there a reason to change it?
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.
my local tests were failing because the var wasn't being read from the install class. Is it a scoping issue with my version of Puppet (4.5)? Agree that it's cleaner that way. Maybe i can work around it in another way
@@ -6,7 +6,7 @@ | |||
$extra_packages_ensure = $sssd::extra_packages_ensure, | |||
) { | |||
|
|||
ensure_packages([$sssd_package],{ ensure => $ensure }) | |||
ensure_packages({$sssd_package => { 'ensure' => $ensure } } ) |
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.
This is causing Puppet 3.8.7 with ruby 1.9.3 to fail in a strange way. need to check on correct syntax as this works on ruby 2.1
This looks good too me. @edestecd do you have anything to add or should we just merge this? |
Everything looks great! Looks like github can squash for us, if you prefer... |
GitHub squash is a good plan. Cheers
|
@dannygoulder thanks for the contribution! |
You're welcome. Thanks for the great work on the module.
|
Needs newer version of puppetlabs-stdlib which I have added to the metadata.json.
Puppetlabs-stdlib 4.x only works with Puppet 3.x which is already a minimum requirement for this module.