Refs #32037: Add parameters to disable qpid services #148
Conversation
There was a problem hiding this comment.
This looks very straightforward.
I don't see a particular need to set defaults via $qpid::{router::,}params::service_{ensure,enable} at this time as I don't see anywhere in our ecosystem where we include either params class. (I don't recall what were the other benefits, if any, of adhering to that pattern)
I wouldn't mind seeing a test case for the proper default behavior, and another test that the defaults can be overridden, but it feels very trivial given the simplicity of this code to hold things up over it. I am assuming your reasoning for not implementing that was probably similar.
| @@ -9,7 +9,7 @@ | |||
| # $acl_file:: File name for Qpid ACL | |||
| # | |||
| # $acl_content:: Content for Access Control List file | |||
|
|
|||
| # | |||
There was a problem hiding this comment.
Nice additional fix that we should have caught earlier
ehelms
force-pushed
the
refs-32037
branch
2 times, most recently
from
9339801
to
05ebea4
Compare
manifests/router/service.pp
Outdated
| ensure => running, | ||
| enable => true, | ||
| ensure => $qpid::router::service_ensure, | ||
| enable => $qpid::router::service_enable, |
There was a problem hiding this comment.
Is it intentional or an oversight that we don't use the same pattern here?
| enable => $qpid::router::service_enable, | |
| enable => pick($qpid::router::service_enable, $qpid::router::service_ensure), |
If there is no reason for the different behavior here, then let's make it consistent.
There was a problem hiding this comment.
Blind addressing of direct comments :) I have updated both to the same pattern now with tests.
| $service_ensure = true, | ||
| $service_enable = undef, |
There was a problem hiding this comment.
I would have liked to see data types here. Ideally whenever you add a parameter, you use a data type.
There was a problem hiding this comment.
Given this class did not have data types already, it was either add it just for these two, add it for all (and possibly overreach the point of the PR) or stick with what was present already. I chose the latter to make the focused changed. And plan to follow up adding types.
No description provided.