should notify only if service is managed #42
Conversation
| } | ||
|
|
||
| if($::uchiwa::manage_services == true){ |
There was a problem hiding this comment.
please just say if $::uchiwa::manage_services { } for style
|
Can you take out the apt-change? This module does actually require apt, and there is a different PR to update it to use the newer puppet-labs-apt api. |
|
Ok, just did iv also made a small if statement change Thanks |
| } | ||
|
|
||
| if $::uchiwa::manage_services { |
There was a problem hiding this comment.
Can you remove this change? It is too specific and operating directly on the package.
It belongs in the init class
Class['Uchiwa::install'] ~> Class['uchiwa::service']
And then let the manage_services gate inside the service class handle whether or not to do anything.
|
Sorry but I don't understand what you mean, if ill remove the if statement the code will fail in cases that the service isn't managed (see https://github.com/Yelp/puppet-uchiwa/blob/master/manifests/install.pp#L44) |
|
Yes, but this logic doesn't belong in the install class. Keep all the "if $manage_services" logic in the service class. Any notifications need to happen in the init class on the entire classes, not just the package resource. Like this pattern: https://www.devco.net/archives/2012/12/13/simple-puppet-module-structure-redux.php |
|
Can you rebase against master if you are still interested in getting this change in? |
No description provided.