Restore behaviour of `servers` and `pools` parameters #103

alexjfisher · 2021-01-04T15:17:34Z

Before the conversion to epp in
#79, an array of
servers/pools would get the iburst option set for each server/pool.

This restores the old behaviour and gets rid of the flatten magic at
the expense of several extra lines of template.

alexjfisher · 2021-01-04T15:21:29Z

This should also get rid of the issue with possible unwanted trailing spaces as described in #94

ekohl

Does it make sense to normalize the array to a hash in a Puppet function and pass that to the template instead to keep it simpler?

templates/chrony.conf.epp

ekohl · 2021-01-04T15:28:31Z

templates/chrony.conf.epp

+<% } -%>
+<% } -%>
+<% if $chrony::pools.is_a(Hash) { -%>
+<% $chrony::pools.keys.sort.each |$pool| { -%>


Can't you use this (untested):

Suggested change

<% $chrony::pools.keys.sort.each |$pool| { -%>

<% $chrony::pools.sort.each |$pool, $pool_options| { -%>

Unfortunately not.

Signature 1 sort(String $string_value, Optional[Callable[2,2]] &$block) Signature 2 sort(Array $array_value, Optional[Callable[2,2]] &$block)

manifests/init.pp

Before the conversion to epp in voxpupuli#79, an array of servers/pools would get the `iburst` option set for each server/pool. This restores the old behaviour and gets rid of the `flatten` magic at the expense of several extra lines of template.

alexjfisher · 2021-01-04T16:27:25Z

Does it make sense to normalize the array to a hash in a Puppet function and pass that to the template instead to keep it simpler?

I've made that change, and I think it's worked out ok.

alexjfisher · 2021-01-04T16:43:47Z

functions/server_array_to_hash.pp

+  if $servers.is_a(Hash) {
+    $servers
+  } else {
+    $servers.reduce({}) |$memo, $server| { # lint:ignore:manifest_whitespace_opening_brace_before


I've added a comment to voxpupuli/puppet-lint-manifest_whitespace-check#8

manifests/init.pp

ekohl · 2021-01-04T16:57:39Z

manifests/config.pp

+        servers => $servers.chrony::server_array_to_hash(['iburst']),
+        pools   => $pools.chrony::server_array_to_hash(['iburst']),
+        peers   => $peers.chrony::server_array_to_hash,


I find this syntax odd. Doesn't this also work?

Suggested change

servers => $servers.chrony::server_array_to_hash(['iburst']),

pools => $pools.chrony::server_array_to_hash(['iburst']),

peers => $peers.chrony::server_array_to_hash,

servers => chrony::server_array_to_hash($servers, ['iburst']),

pools => chrony::server_array_to_hash($pools, ['iburst']),

peers => chrony::server_array_to_hash($peers),

I guess for some functions it reads better (eg. reduce with an optional start value), but here, I think you're right. I've updated it.

ekohl · 2021-01-04T16:59:29Z

spec/classes/chrony_spec.rb

+              'server 2.pool.ntp.org iburst',
+              'server 3.pool.ntp.org iburst'
+            ]
+            expect(config_file_contents.split("\n") & expected_lines).to eq(expected_lines)


Would this also make sense?

it do is_expected.to contain_file(config_file). with_content(/server 0.pool.ntp.org iburst/). with_content(/server 1.pool.ntp.org iburst/). with_content(/server 2.pool.ntp.org iburst/). with_content(/server 3.pool.ntp.org iburst/) end

I think it does essentially the same, but feels more native.

I'm specifically testing the lines have been sorted and appear in the correct order.

I've had some issues with & expected_lines in the past. For example, when I wanted to match something that showed up multiple times (like } in a block). https://github.com/rspec/rspec-expectations#collection-membership isn't very satisfying either.

Another is to have a large text block:

let(:expected_content) do <<~CONTENT server ... CONTENT end it { is_expected.to contain_file(config_file).with_content(Regex.new(Regex.escape(expected_content))) }

No idea if it works, but I'm wondering what a good solution is here with voxpupuli/voxpupuli-test#14 in mind.

I've left this as is for now. I think the shared matchers will be really useful for getting a consistent approach in the future though.

ekohl · 2021-01-04T17:01:26Z

templates/chrony.conf.epp

-<% $chrony::peers.each |$peer| { -%>
-peer <%= $peer.flatten.join(' ') %>
+<% $peers.keys.sort.each |$peer| { -%>
+<%   if $peers[$peer] and !$peers[$peer].empty { -%>


Is this the same?

Suggested change

<% if $peers[$peer] and !$peers[$peer].empty { -%>

<% if !empty($peers[$peer]) { -%>

If so, I would probably reverse the conditions so it becomes if empty() {} else {}.

I don't think you can call empty on undef though? I'll double check.

https://puppet.com/docs/puppet/6.4/function.html#empty states:

For backwards compatibility with the stdlib function with the same name the following data types are also accepted by the function instead of raising an error. Using these is deprecated and will raise a warning:

Numeric - false is returned for all Numeric values.
Undef - true is returned for all Undef values.

So yes, you can call it but you probably shouldn't is the answer?

I'm ok with a warning (and I've already made the change!). I don't think many users will encounter this and TBH, supporting undef is a corner case that's easily avoided by the user. eg the hiera in #94 (comment)
could be changed to

chrony::servers: ntp1: - minpoll 7 ntp2: []

puppetlabs/puppet#6776 (comment) ;)

The deprecation is removed in this PR.

Simpify the epp template by using a private helper function to normalise arrays into hashes for the `servers`, `pools`, and `peers` parameters.

Used by `servers`, `pools`, and `peers`, it allows either an array of hosts, or a hash of hosts with their respective options.

functions/server_array_to_hash.pp

ekohl

Some questions inline, but overall 👍 and wouldn't mind merging it as is.

ekohl · 2021-01-05T16:08:14Z

REFERENCE.md

@@ -17,6 +17,19 @@
 * `chrony::params`: chrony class parameters
 * `chrony::service`: Manages the chrony service

+### Functions
+
+#### Public Functions


Perhaps worth a bug report to puppet-strings that it shouldn't output this header if there are no public functions.

I've opened puppetlabs/puppet-strings#267

ekohl · 2021-01-05T16:08:55Z

REFERENCE.md


 This selects the servers to use for NTP servers.  It can be an array of servers
-or a hash of servers to their respective options.
+or a hash of servers to their respective options. If an array is used, `iburst` will be configured for each server.
+If you don't want to use `iburst`, use a hash instead.


Perhaps an @example is useful for this.

ekohl · 2021-01-05T16:10:00Z

templates/chrony.conf.epp

-<% $chrony::servers.each |$server| { -%>
-server <%= $server.flatten.join(' ') %>
+<% $servers.keys.sort.each |$server| { -%>
+<%   if $servers[$server].empty { -%>


This is a lack of knowledge on my side, but does this work or should it be:

Suggested change

<% if $servers[$server].empty { -%>

<% if $servers[$server].empty() { -%>

Both are fine. If using parenthesis, I'd have probably written if empty($servers[$server]), but I quite like the first form.

ekohl · 2021-01-05T16:10:28Z

types/servers.pp

@@ -0,0 +1,4 @@
+type Chrony::Servers = Variant[


Is there some documentation that makes sense for this type?

Yes. I'll add that. Puppet-strings style-guide wrongly states "However, Strings does not generate information for type aliases or facts"...

puppetlabs/puppet-strings#199 (comment)

alexjfisher force-pushed the restore_servers_and_pools_behaviour branch from bb6a43b to c2adfde Compare January 4, 2021 15:19

alexjfisher mentioned this pull request Jan 4, 2021

Fix template to not add trailing space after servers,peers,pools #94

Closed

ekohl reviewed Jan 4, 2021

View reviewed changes

alexjfisher force-pushed the restore_servers_and_pools_behaviour branch from c2adfde to 2b78b11 Compare January 4, 2021 15:45

alexjfisher force-pushed the restore_servers_and_pools_behaviour branch from 2b78b11 to 58b35e7 Compare January 4, 2021 15:52

alexjfisher force-pushed the restore_servers_and_pools_behaviour branch from b4539e8 to 9e64c25 Compare January 4, 2021 16:32

alexjfisher commented Jan 4, 2021

View reviewed changes

alexjfisher requested a review from ekohl January 4, 2021 16:48

ekohl reviewed Jan 4, 2021

View reviewed changes

Simplify template with helper function

9fff7a6

Simpify the epp template by using a private helper function to normalise arrays into hashes for the `servers`, `pools`, and `peers` parameters.

alexjfisher force-pushed the restore_servers_and_pools_behaviour branch from 9e64c25 to 9fff7a6 Compare January 4, 2021 17:53

Add Chrony::Servers type alias

a567bca

Used by `servers`, `pools`, and `peers`, it allows either an array of hosts, or a hash of hosts with their respective options.

alexjfisher requested a review from ekohl January 4, 2021 18:50

b4ldr reviewed Jan 5, 2021

View reviewed changes

functions/server_array_to_hash.pp Show resolved Hide resolved

ekohl approved these changes Jan 5, 2021

View reviewed changes

Add some more examples to docs

d84519d

alexjfisher merged commit 21f0250 into voxpupuli:master Jan 5, 2021

alexjfisher added the bug Something isn't working label Jan 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore behaviour of `servers` and `pools` parameters #103

Restore behaviour of `servers` and `pools` parameters #103

alexjfisher commented Jan 4, 2021

alexjfisher commented Jan 4, 2021

ekohl left a comment

ekohl Jan 4, 2021

alexjfisher Jan 4, 2021

alexjfisher commented Jan 4, 2021

alexjfisher Jan 4, 2021

ekohl Jan 4, 2021

alexjfisher Jan 4, 2021

ekohl Jan 4, 2021

alexjfisher Jan 4, 2021

ekohl Jan 4, 2021

alexjfisher Jan 4, 2021

ekohl Jan 4, 2021

alexjfisher Jan 4, 2021

ekohl Jan 4, 2021

alexjfisher Jan 4, 2021

alexjfisher Jan 4, 2021 •

edited

Loading

ekohl left a comment

ekohl Jan 5, 2021

alexjfisher Jan 5, 2021

ekohl Jan 5, 2021

alexjfisher Jan 5, 2021

ekohl Jan 5, 2021

alexjfisher Jan 5, 2021

ekohl Jan 5, 2021

alexjfisher Jan 5, 2021

alexjfisher Jan 5, 2021

	<% $chrony::pools.keys.sort.each \|$pool\| { -%>
	<% $chrony::pools.sort.each \|$pool, $pool_options\| { -%>

	<% if $peers[$peer] and !$peers[$peer].empty { -%>
	<% if !empty($peers[$peer]) { -%>

	<% if $servers[$server].empty { -%>
	<% if $servers[$server].empty() { -%>

Restore behaviour of servers and pools parameters #103

Restore behaviour of servers and pools parameters #103

Conversation

alexjfisher commented Jan 4, 2021

alexjfisher commented Jan 4, 2021

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjfisher commented Jan 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjfisher Jan 4, 2021 • edited Loading

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Restore behaviour of `servers` and `pools` parameters #103

Restore behaviour of `servers` and `pools` parameters #103

alexjfisher Jan 4, 2021 •

edited

Loading