Consolidate templates and convert to epp() #79

chrekh · 2020-07-09T09:44:41Z

Replace the separate erb-templates for chrony.conf with one epp-template.
The chrony.conf.epp is based on the erb-template for redhat.

ekohl

I don't use chrony myself so can't comment that much. Just small things that jumped out to me

ekohl · 2020-07-09T09:47:25Z

manifests/params.pp

@@ -6,7 +6,7 @@
    'Archlinux' : {
      $cmdacl            = ['cmdallow 127.0.0.1']
      $config            = '/etc/chrony.conf'
-      $config_template   = 'chrony/chrony.conf.archlinux.erb'
+      $config_template   = 'chrony/chrony.conf.epp'


Since this is now the same everywhere, you can move it to init.pp as a static value.

Yes absolutely, but I think it should have it's own commit, and a separate PR

I disagree. init.pp already has all static values and params.pp only has values that differ for various OSes.

Ok, I see your point I have added a commit to do this.

templates/chrony.conf.epp

Replace the separate erb-templates for chrony.conf with one epp-template. The chrony.conf.epp is based on the erb-template for redhat.

Fix typo cut&paste typo Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>

Since this now has the same value for all OS, this don't need to be set by params.pp

ekohl

From a Puppet code perspective this looks ok. I don't use chrony so I can't comment too much on anything you may have missed.

Any chrony user can take a look?

ekohl · 2020-07-09T10:39:59Z

templates/chrony.conf.epp

+bindcmdaddress <%= $addr %>
+<%   } -%>
+<% } -%>
+<% if ! $chrony::cmdacl.empty { -%>


Technically this isn't needed. It's guaranteed to be an array and you can iterate 0 times over an empty array. In other cases it makes sense for whitespace control but that doesn't happen here.

You are right again. And you also guessed correctly that my reason for several of the if-statements is to limit empty blank lines in the file.

And also decrease nr of empty lines added to configfile.

aboe76

Nice work, this will make the maintenance easier

ekohl

Thanks!

chrekh · 2020-07-20T11:17:57Z

templates/chrony.conf.epp

+# NTP servers
+<% $chrony::servers.each |$server| { -%>
+server <%= $server.flatten.join(' ') %>
+<% } -%>


This is not good enough when $chrony::servers is a hash. It (kind of) works, because
.each() returns a Array ['key','value']. But if I use hieradata with a hash without value,
this will result in a line ending with a space.

Example:

chrony::servers: ntp1.example.com: - minpoll 3 - maxpoll 7 ntp2.example.com:

It results in server "ntp1.example.com minpoll 3 maxpoll 7", but "server ntp2.example.com "

I'm going to think some about this, and submit a PR with a fix later. We should also have
a test that would have detected this.

The conversion from ERB to EPP in voxpupuli#79 changed the behavior of queryhosts slightly. Before, setting queryhosts to the empty string worked for configuring chrony to allow any query. After, empty string is considered empty, so the "allow" directive would not be rendered. Enforcing the data type ensures that people upgrading this module who have been using empty string get an error rather than a valid but misconfigured chrony.conf. The replacement for empty string is an array with a single empty string element: ['']

The conversion from ERB to EPP in #79 changed the behavior of queryhosts slightly. Before, setting queryhosts to the empty string worked for configuring chrony to allow any query. After, empty string is considered empty, so the "allow" directive would not be rendered. Enforcing the data type ensures that people upgrading this module who have been using empty string get an error rather than a valid but misconfigured chrony.conf. The replacement for empty string is an array with a single empty string element: ['']

alexjfisher · 2021-01-04T10:10:14Z

templates/chrony.conf.epp

@@ -0,0 +1,122 @@
+# NTP servers
+<% $chrony::servers.each |$server| { -%>
+server <%= $server.flatten.join(' ') %>


If $chrony::servers is an array, this is broken.
Error: Error while evaluating a Method call, flatten(): Requires array to work with

Also the old behavior was to use iburst

<% if @servers.is_a?(Hash) then @servers.keys.sort.each do |server| -%> server <%= server %> <%= @servers[server].join(' ') %> <% end else Array(@servers).each do |server| -%> server <%= server %> iburst <% end end -%>

Actually... it might just be a problem because I was testing against a puppet 4 system... :(

Prior to puppet 5.5, flatten comes from stdlib.

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.

This updates the refclock parameter data types to be a hash which matches the examples and updates the template to properly generate multiple refclock entries. This looks to have broken via a combination of voxpupuli#141 and voxpupuli#79. Rather than restore the full variations that were supported in voxpupuli#79 a simplified hash structure was chosen. Fixes voxpupuli#189 Signed-off-by: Ben Magistro <[email protected]>

This updates the refclock parameter data types to be a hash which matches the examples and updates the template to properly generate multiple refclock entries. This looks to have broken via a combination of voxpupuli#141 and voxpupuli#79. Rather than restore the full variations that were supported prior to voxpupuli#79 a simplified hash structure was chosen. Fixes voxpupuli#189 Signed-off-by: Ben Magistro <[email protected]>

This was referenced Jul 9, 2020

Consolidate templates #67

Closed

Fix refclocks #78

Closed

ekohl reviewed Jul 9, 2020

View reviewed changes

Consolidate templates and convert to epp()

b971db0

Replace the separate erb-templates for chrony.conf with one epp-template. The chrony.conf.epp is based on the erb-template for redhat.

chrekh force-pushed the epp branch from 27ab59e to b971db0 Compare July 9, 2020 10:05

Update templates/chrony.conf.epp

59f61aa

Fix typo cut&paste typo Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>

ekohl linked an issue Jul 9, 2020 that may be closed by this pull request

Consolidate templates #67

Closed

Move default value for $config_template from params.pp to init.pp

aa4be5a

Since this now has the same value for all OS, this don't need to be set by params.pp

ekohl approved these changes Jul 9, 2020

View reviewed changes

Remove unneded condition for Array $chrony::cmdacl

1defd56

And also decrease nr of empty lines added to configfile.

aboe76 approved these changes Jul 10, 2020

View reviewed changes

ekohl approved these changes Jul 10, 2020

View reviewed changes

ekohl merged commit f65cccf into voxpupuli:master Jul 10, 2020

chrekh deleted the epp branch July 10, 2020 12:59

chrekh mentioned this pull request Jul 11, 2020

peer parameter doesn't do anything on ArchLinux #57

Closed

ekohl linked an issue Jul 11, 2020 that may be closed by this pull request

peer parameter doesn't do anything on ArchLinux #57

Closed

ekohl mentioned this pull request Jul 11, 2020

avoid changing configuration when adding optional parameters #64

Closed

ekohl linked an issue Jul 11, 2020 that may be closed by this pull request

avoid changing configuration when adding optional parameters #64

Closed

chrekh commented Jul 20, 2020

View reviewed changes

kenyon mentioned this pull request Nov 16, 2020

queryhosts: enforce Array[String] data type #101

Merged

alexjfisher reviewed Jan 4, 2021

View reviewed changes

alexjfisher mentioned this pull request Jan 4, 2021

Restore behaviour of servers and pools parameters #103

Merged

bmagistro mentioned this pull request Jan 4, 2024

init: change refclocks data type to Hash #190

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate templates and convert to epp() #79

Consolidate templates and convert to epp() #79

chrekh commented Jul 9, 2020

ekohl left a comment

ekohl Jul 9, 2020

chrekh Jul 9, 2020

ekohl Jul 9, 2020

chrekh Jul 9, 2020

ekohl left a comment

ekohl Jul 9, 2020

chrekh Jul 9, 2020

aboe76 left a comment

ekohl left a comment

chrekh Jul 20, 2020

alexjfisher Jan 4, 2021

alexjfisher Jan 4, 2021

Consolidate templates and convert to epp() #79

Consolidate templates and convert to epp() #79

Conversation

chrekh commented Jul 9, 2020

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

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

aboe76 left a comment

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