fix rewrite rules being ignored

[trefzer]

In 7b22dae the default value for $rewrites was changed from undef to [] in manifests/vhost.pp. This results in templates_vhost/_rewrite.erb always evaluating to false (boolean check on an empty array variable is still true) unless $rewrites is explicitly set to undef. This results in the content of $rewrite_rule being ignored.

closes: #2318

[CLAassistant]

All committers have signed the CLA.

[ekohl]

This looks correct to me, but it'd be great if we have a test case to prevent future regressions.

[trefzer]

@ekohl
I agree, but since the logic is in a template we would need to check the final content in the file which I have no idea how to do with spec test. (anyone who nows please let me know !)

[ekohl]

I would probably take this example:

puppetlabs-apache/spec/defines/vhost_spec.rb

Lines 1723 to 1736 in 77255d6

	context 'empty rewrites_with_rewrite_inherit' do
	let(:params) do
	super().merge(
	'rewrite_inherit' => true,
	'rewrites' => [],
	)
	end
	it {
	is_expected.to contain_concat__fragment('rspec.example.com-rewrite').with(
	content: %r{^\s+RewriteOptions Inherit$},
	)
	}
	end

Then copy it and pass some parameters that triggers the behavior you want to test.

Note you can also test the whole file:

it {
  is_expected.to contain_concat__fragment('rspec.example.com-rewrite').with_content <<~CONTENT
    Here you can write the full content
    And use multiple lines
  CONTENT
}

[ekohl]

You can't test this way (as the action indicates), but you can chain it: .with(content: ...).with(content: ...)

[trefzer]

done

[david22swan]

@trefzer Hey sorry to bother you, just checking in on when you'll have time to finish with the sped test?

[ekohl]

These changes should make the test suite work.

[ekohl]

Suggested change

	.with_content(%r{^\s+RewriteOptions Inherit$}),
	.with_content(%r{^\s+RewriteBase /}),
	.with_content(%r{^\s+RewriteRule ^index\.html$ welcome.html$}),
	.with_content(%r{^\s+RewriteOptions Inherit$})
	.with_content(%r{^\s+RewriteBase /})
	.with_content(%r{^\s+RewriteRule ^index\.html$ welcome.html$})

[trefzer]

ups :) missed them.

[ekohl]

Suggested change

	<%- if @rewrite_inherit or not @rewrites.empty? -%>
	<%- if @rewrite_inherit or !@rewrites.empty? -%>

[ekohl]

This should make it valid Ruby

Suggested change

)

[david22swan]

LGTM

@ekohl Just checking you're good with the changes you requested?

If so will merge in and release

[ekohl]

Yes, I'm good. All my comments were there to make the test suite pass (we'll ignore mend for now) so 👍 to merging this.

Thanks @trefzer for your efforts! And it was great to collaborate on a PR in person again. That was a long time ago for me

[david22swan]

Good good good

Should be released by end of day