-
Notifications
You must be signed in to change notification settings - Fork 328
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
WIP: Refactoring of NTP module #66
Conversation
| } else { | ||
| fail('autoupdate parameter must be true or false') | ||
| } | ||
| Class['::ntp::install'] -> Class['::ntp::config'] ~> Class['::ntp::service'] |
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 particular change will require an anchor resource otherwise require ntp is probably not going to work as expected (class containment per 8040).
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.
You're totally right, I meant to add that in already but got distracted. :)
| $restrict = true | ||
|
|
||
| if $autoupdate { | ||
| notice('autoupdate parameter has been deprecated and replaced with package_ensure. Set this to latest for the same behavior as autoupdate => true.') |
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.
isn't this always going to give a deprecation as the module stands today? is $autoupdate intended to be undef and should this be in the init.pp file?
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.
Not unless things changed, if $blah {} won't match if $blah is false by default, doesn't have to be undef. I double checked this with 3.2 but I haven't gone back to make sure it wasn't a change in behavior along the way yet! :)
I did however move it to init.pp, you're right, it makes more sense there.
Add the anchor pattern. Move the deprecation warning out of params to init.
manage_service param.
few missing gaps in the testing.
|
Alright, a bunch of changes in the latest batch, hopefully this brings us closer to where we'd like to see this. Specs now cover all the classes individually, the documentation (well, README) is fixed up and the params have all been named sensibly. This afternoon I'm going to be working on rspec-system spec tests for this one and then we can talk about releasing it. |
ntp on the child classes.
being installed and the config file at least makes basic sense.
|
Sorry for the delay; just tested on Gentoo, and it appears to be working just fine. 👍 |
|
@tianon Awesome, glad to hear things are working good! I think I'm pretty close to releasing this at this point so it can get wider testing. |
TODO:
Add rspec-system tests.