-
-
Notifications
You must be signed in to change notification settings - Fork 146
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
move params to data-in-modules #67
Conversation
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 this doesn't really add anything. We're not using params.pp and that's very clear. It's easy to follow. Data in modules IMHO only makes sense when you have a big case statement in params.pp with nested exceptions for some versions.
systemd::networkd_ensure: 'running' | ||
systemd::manage_timesyncd: false | ||
systemd::timesyncd_ensure: 'running' | ||
systemd::ntp_server: ~ |
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'm not sure what the best practice is, but I think it's cleaner to have any Optional
type in init.pp
default to undef
.
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.
AFAIK best practice is ~, which is also used in the puppetlabs modules.
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 guess it would be more consistent to have all parameters in hiera.
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.
Oh, I now see #65 where it does make sense.
LGTM, but I think there should be a warning in the README that you need to use the Hiera 5 format to use the module from then on. |
6ab5b22
to
4b1e52d
Compare
Emtpy hiera files throw puppet 4 warnings
This got pulled out of #65. The lowest recommended puppet version for this is 4.10.10. In the Vox Pupuli namespace we had a lot of argumentation if this requires a major version bump or not. From a point of defensive programming this should be a major bump.