Finish conversion to EPP templates

[smortex]

A lot of work was done to convert the module templates form ERB to EPP, but a few templates where still to be converted.

Along with various benefits, EPP templates offer better detection for access to undefined variables. This refactoring therefore fix a few issues that where reported while converting. Also a bunch of outdated comments about which template use which variable where removed no that this usage is explicit.

The extensive test suite helped ensure the conversion was not introducing regressions.

[smortex]

This change should be backwards-compatible. See related commit 675fb82 for details.

[Dan33l]

Huge PR. I tried to add my eyes, and added an inline comment.

[binford2k]

Hallo @smortex! Fill out this form if you'd like to claim your May the Source Be With You sticker! https://forms.office.com/r/Cn55uJmWMH

[ekohl]

I wish we had more type enforcement in templates and EPP allows that. If this is backwards incompatible anyway, it may be a good moment to do so.

[ekohl]

I think we shouldn't pass arguments prefixed with _, so would prefer:

Suggested change

	'_secdefaultaction' => $_secdefaultaction,
	'secdefaultaction' => $_secdefaultaction,

I know other places do this wrong now, so I'd be OK with leaving that for later.

[smortex]

I decided to go this way in order to limit the changes to just ERB -> EPP. IMHO, we should pass secdefaultaction => $secdefaultaction and move the code that does the transformation into the template, but it would be hard to track the move with all other changes, and as you say this kind of fix would be welcome in other places.

So for now, my proposal is to keep it that way.

[ekohl]

If we already have a hash, the comment is redundant

Suggested change

# Template uses $extended_status, $status_path

[ekohl]

Isn't this equal to:

Suggested change

	<% $_requires = if $requires { $requires } else { $requires_defaults} -%>
	<% $_requires = pick($requires, $requires_defaults) -%>

But then I'd prefer to have that on a higher level (where epp() is called).

[ekohl]

Unrelated, but wtf is String(type($_requires, 'generalized')).index('Array') == 0? Isn't that equal to $_requires =~ Array?

Would it make sense to also use native types to enforce a structure? I think the resulting template is easier to understand:

<%- | Variant[
        String[0],
        Array[String[1]],
        Struct[
          {
            enforce  => Optional[Enum['all', 'none', 'any']],
            requires => Array[String[1]],
          },
        ],
      ] $requires,
| -%>
<% if $requires.downcase in ['', 'unmanaged'] { -%>
<% elsif $requires =~ String { -%>
    Require <%= $requires %>
<% } elsif $requires =~ Array -%>
  <%- $requires.each |$req| { -%>
    Require <%= $req %>
  <%- } -%>
<% } $requires =~ Hash { -%>
  <%- if $requires['enforce'] { -%>
    <%- $enforce_str = "Require${requires['enforce'].capitalize}>\n" -%>
    <%- $enforce_open = "    <${enforce_str}" -%>
    <%- $enforce_close = "    </${enforce_str}" -%>
    <%- $indentation = '    ' -%>
  <%- } else { -%>
    <%- $enforce_open = '' -%>
    <%- $enforce_close = '' -%>
    <%- $indentation = '' -%>
  <%- } -%>
<%# %><%= $enforce_open -%>
      <%- $requires['requires'].each |$req| { -%>
<%# %>    <%= $indentation -%>Require <%= $req %>
      <%- } -%>
<%# %><%= $enforce_close -%>
<% } -%>

I'd even be tempted to rewrite the last bit to avoid variables:

<% } $requires =~ Hash { -%>
  <%- if $requires['enforce'] { -%>
    <Require${requires['enforce'].capitalize}>
      <%- $requires['requires'].each |$req| { -%>
        Require <%= $req %>
      <%- } -%>
    </Require${requires['enforce'].capitalize}>
  <%- } else { -%>
      <%- $requires['requires'].each |$req| { -%>
    Require <%= $req %>
      <%- } -%>
  <%- } -%>
<% } -%>

[ekohl]

So this would become:

Suggested change

	<%= epp("apache/mod/_require.epp", { 'requires' => $requires, 'requires_defaults' => $requires_defaults }) -%>
	<%= epp("apache/mod/_require.epp", { 'requires' => pick($requires, $requires_defaults) }) -%>

[ekohl]

Isn't hash sorting always by key anyway?

[smortex]

Unfortunately, the sort function does not accept Hash, only Array or String. I though a second about adding this to Puppet, but that would require new versions of Puppet and might be an issue…

[ekohl]

I think you can avoid the additional variable with little loss of readability here

Suggested change

	<%- if $flag.downcase =~ /true|yes|on|1/ { $_flag = 'on' } else { $_flag = 'off' } -%>
	php_flag <%= $key %> <%= $_flag %>
	php_flag <%= $key %> <%= if $flag.downcase =~ /true|yes|on|1/ { 'on' } else { 'off' } %>

[ekohl]

Given the repetition, perhaps a helper would be useful? There's a lot of overlap with $_php.epp anyway so other comments I had there apply here too.

[ekohl]

Can we move the empty() check to vhost.pp and only include the concat fragment if it's non-empty? In fact, we already have $use_env_mod = !empty($setenv) so you can reuse that variable.

[smortex]

Yeah, I saw that but kept it as it was before. I will add a commit to avoid the double-test.

[ekohl]

Like _setenv.epp, I'd prefer to keep the if in the Puppet code and avoid rendering the whole template.