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

[chrekh]

The attempt to simplify the template by letting flatten() take care of the different
allowed data-types for servers,peers, and pools, was unfortunately a bit too
optimistic. The result was that join(' ') on a Array with empty element created a extra
space caracter when $chrony::servers was a hash with a key but no value.

[ekohl]

Could you add some tests to cover these data types?

[ekohl]

Does that work? I thought it would be is_a?. Isn't this more native Puppet syntax:

Suggested change

	<% if $chrony::servers.is_a(Hash) { -%>
	<% if $chrony::servers =~ Hash { -%>

[chrekh]

is_a? is a ruby function, is_a is a function from stdlib, equivalent to =~. I just
think the object-oriented call looks nicer, and when I se a =~ I'm thinking regexp. But
if you want to I can change it.

[ekohl]

It's a bit confusing to me because I'm so used to ERB templates where it would be invalid. Not sure what the best practice would be here. Given the repetition, perhaps a private function chrony::parameter_as_hash() would be useful to simplify the template? https://puppet.com/docs/puppet/6.17/lang_iteration.html#using-iteration-to-transform-data suggests you can do this with pure Puppet.

[chrekh]

This could work.

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

i.e. Use flatten to take care of the permitted types, and use filter to remove the
resulting empty element before joining.

[chrekh]

Could you add some tests to cover these data types?

I think we should add a test .without_content(%r{\s$}) to catch this problem, and
similar. But since #89 is changing the tests a lot, I planned to wait with that until it's
merged (or rejected).

[chrekh]

This is another attempt. This time the goal is to keep the template simpler and avoid the
need for different code for hash than for array.

I also tried to add a test to detect this, but failed. .without_content(%r{\s$}) didn't
work because it matches '\n\n' also. I also tried to recreate the hash value created by
hieradata

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

so i could test with .with_content(%r{^\s*server ntp2$}) }. But I failed

servers: { 'ntp1' => ['minpoll 7'], 'ntp2' => undef } is syntax error.
servers: { 'ntp1' => ['minpoll 7'], 'ntp2' } is syntax error.
servers: { 'ntp1' => ['minpoll 7'], 'ntp2' => nil } is valid syntax, but results in server ntp2 nil in chrony.conf

[ekohl]

servers: { 'ntp1' => ['minpoll 7'], 'ntp2' => nil } is valid syntax, but results in server ntp2 nil in chrony.conf

The problem is that you have a filter $value != undef but $value is ['ntp2', nil].

[chrekh]

Yes, but I fail to figure out how to test, or more precise how to recreate the content in
the hash in chrony_spec.rb

But I realize now that when I saw the resulting server ntp2 nil I was actually testing
with the added test to master. And the test is supposed to fail. What troubled me was that
I expected to see server ntp2 (trailing space).

When I try the new test on top of this branch the test passes. So perhaps it's usable
anyway. I'll add a commit with the test and push it here, and let you decide.

[chrekh]

No, that was wrong. In the previous comment I was lazy and only added the test to the
first osfamily, and it passed. Now I see why. It has when 'Archinux' and is never
run. :)

I'll think some more about this, and let you know if I find a solution.

[chrekh]

No, I fail no matter what I try.

With a hash in the test-code with servers: { 'ntp1' => ['minpoll 7'], 'ntp2' => nil }

And with hieradata

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

1. With filter { $value != nil } in template the test passes, but when run I get the trailing space server ntp2

2. With filter { $value } or { $value != undef } in the template he testcase produces server ntp2 nil and fails.

3. With filter { !($value == nil or $value == undef) } in the template, the test succeds and I get a file without trailing space.

But I don't like that at all. It feels like that filtering undef is the real solution for
the real problem, and adding filter for nil is just to please the test, and then the test
isn't really useful anyway.

[alexjfisher]

I also tried to add a test to detect this, but failed. .without_content(%r{\s$}) didn't
work because it matches '\n\n' also. I also tried to recreate the hash value created by
hieradata

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

FWIW, in rspec-puppet, you can use the symbol :undef. I'm not 100% we should even allow undef, as well as an empty array, but #103 should work with both.

[chrekh]

This is no longer current or relevant.