Use double, vs single quotes around add_header values (#991)

[wyardley]

Not sure how

https://github.com/voxpupuli/puppet-nginx/blob/master/spec/defines/resource_server_spec.rb#L1197-L1227
passes either before / after, but this seems to pass tests. I'm going to mark this as in-progress, though.

[wyardley]

TODO: also need to add a test case where a value has single quotes in it, e.g.,
"default-src 'self';"
Of course, there could be the reverse case where there are double-quotes, so not sure how "smart" we want to get about this in code.

[wyardley]

@xaque208 @bastelfreak does this change make sense to you all?
And any ideas why the current test passes in either case when it seems like it shouldn't pass either way (because it's just matching \s+ in between with no literal single or double quotes)?

[zachfi]

@wyardley This looks like the tests are lying to us. That %r{} style I've not seen before, but I suspect that it might be incorrectly used and could give false results.

[wyardley]

@xaque208 IIRC, we use %r{} because the Voxpup Rubocop conventions dictate it. I don't think it's that different from a normal regex, except that you don't have to escape certain things (slashes, for example). I don't think it should be causing a problem in this case, though.

For example (I'm not a Ruby person, so sorry if this is not the best or most idiomatic):

#!/usr/bin/env ruby

string = 'test1 "test test" test3'
regexp = Regexp.new(%r{test1\s+test test\s+test3})
puts regexp.match(string)
regexp2 = Regexp.new(%r{test1\s+"test test"\s+test3})
puts regexp2.match(string)

returns

% ./test2.rb

test1 "test test" test3

[wyardley]

@rnelson0 @bastelfreak Any thoughts on the tests issue? Also, more generally, does anyone have an opinion about whether there are any cases where the change to double quotes would cause a problem here?

[rnelson0]

I don't see how it would matter here, since this is inside the plain text portion of the ERB, not the interpreted portion. Because we don't know how it is passing, I would change either L1199 or L1205-1207 so they do not match and see how that behaves.

[wyardley]

Seems to still pass for me even if I change

puppet-nginx/spec/defines/resource_server_spec.rb

Line 1199 in 1d903c1

default_params.merge(add_header: { 'header3' => 'test value 3', 'header2' => 'test value 2', 'header1' => 'test value 1' })

to

      context 'when add_header is set' do
        let :params do
          default_params.merge(add_header: { 'header3' => 'test value 3', 'header2' => 'test value 2', 'header1' => 'asdfsadfdafsafdsfads value 1' })

[rnelson0]

Wow, that's amazing, clearly a bad test.

[mterzo]

Rubocop was updated and replaced a config option

From their changelog.

#3737: Add new Style/MethodCallWithArgsParentheses cop. (@dominh)
Renamed MethodCallParentheses to MethodCallWithoutArgsParentheses. (@dominh)

https://github.com/bbatsov/rubocop/pull/3797/files

[wyardley]

@xaque208 looking at it more closely, this does seem kind of odd to me; not totally getting why there are the nested %r expressions:

with_content(%r{
            %r|
            [...]
            |})

Having it all on one line does seem to work, though it's messy and make a long line (but does pass rubocop)

          is_expected.to contain_concat__fragment("#{title}-header").with_content(%r{\s+add_header\s+"header1" "test value 1";\n\s+add_header\s+"header2" "test value 2";\n\s+add_header\s+"header3" "test value 3";\n})

[wyardley]

        it 'has correctly ordered entries in the config' do
          is_expected.to contain_concat__fragment("#{title}-header").with_content(%r{
            \s+add_header\s+"header1"\s"test\svalue\s1";\n
            \s+add_header\s+"header2"\s"test\svalue\s2";\n
            \s+add_header\s+"header3"\s"test\svalue\s3";\n
          }x)

This will also work, though not sure if it's an improvement in terms of readability.

[wyardley]

I'm going to update it to use the test that should actually work, even if it could be formatted more nicely... if folks have suggestions on how to do it in a prettier way, let me know.

[zachfi]

@wyardley This looks better to me.