On Wed, Jul 10, 2013 at 5:14 AM, Matthias Saou <matth...@saou.eu> wrote:
>
> It looks really nice and clean! I would personally only have a few very
> minor nitpicks :
>
> * Shouldn't the placeholder files/README.markdown be removed?
> * Space between class name and parenthesis inconsistency :
> "class foo(..." vs. "class foo (..."
> * In the templates, this comment should use single quotes :
> # Managed by puppet class { "ntp": servers => [ ... ] }
> * In the el template, this variable isn't in the current scope :
> <% if @is_virtual == "false" -%>
> Add a new params variable for it, similar to $panic?
> * For "real" RHEL, the ntp server hostnames used will be "centos"
> instead of the original "rhel" ones. I'm not sure this is worth
> trying to fix, though.
>
> Great work on cleaning up the module!
Good eye! I've opened
https://github.com/puppetlabs/puppetlabs-ntp/pull/68for these fixes.
I just switched over to scope.lookupvar('::is_virtual')
for that single use rather than make it a parameter as it's coming out of
the box via facter.
If anyone knows the "official" rhel ntp servers then let me know and I'll
distinguish between rhel/centos if anyone cares. Honestly we'd be better
off just defining a pool from pool.ntp.org as the "standard" set used
everywhere and let people override them rather than carry this list around.
It's less likely to drift out of date. I started another email about
unifying the templates so if anyone wants to discuss pools, lets take it
there.
--
You received this message because you are subscribed to the Google Groups
"Puppet Users" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to puppet-users+unsubscr...@googlegroups.com.
To post to this group, send email to puppet-users@googlegroups.com.
Visit this group at http://groups.google.com/group/puppet-users.
For more options, visit https://groups.google.com/groups/opt_out.
