Add fastcgi_param parameter to vhost resource

[zachfi]

Without this change, specifying fastcgi parameters are not possible on
the vhost. This change modifies vhost resource to allow passing the
fastcgi_param to the location resource directly, as is the case with a
few of the other fastcgi related parameters.

[wyardley]

This seems sane to me, but you'll need to add tests in spec/defines/resource_vhost_spec.rb (and then squash commits).
I think they'd be very similar to:
https://github.com/voxpupuli/puppet-nginx/blob/master/spec/defines/resource_location_spec.rb#L503-L535
though the 'without_content' bits should probably be looser, that is, just look for
without_content(%r{^\s+fastcgi_param\s+} or something, rather than
without_content(%r{^[ ]+fastcgi_param\s+SCRIPT_FILENAME\s+.+?;})

[zachfi]

Tests added. Let me know if thats not enough.

[zachfi]

Hmm, I'm not used to rubocop. Any idea what I've done wrong? The log doesn't load here.

[wyardley]

@xaque208 Yeah, it can be kind of a hassle. Basically, before pushing, try to run:
bundle install (if hasn't already been run)
bundle exec rake spec_clean
bundle exec rake spec_prep
bundle exec rake rubocop
bundle exec rake release_checks

Currently, the complaints I see are:

Running RuboCop...
/private/tmp/puppet-nginx/.rubocop.yml: Metric/BlockLength has the wrong namespace - should be Metrics
Inspecting 20 files
................C...

Offenses:

spec/defines/resource_vhost_spec.rb:953:47: C: Style/SpaceInsideHashLiteralBraces: Space inside { missing.  (https://github.com/bbatsov/ruby-style-guide#spaces-operators)
          default_params.merge(fastcgi_param: {'key' => 'value'})
                                              ^
spec/defines/resource_vhost_spec.rb:953:64: C: Style/SpaceInsideHashLiteralBraces: Space inside } missing.  (https://github.com/bbatsov/ruby-style-guide#spaces-operators)
          default_params.merge(fastcgi_param: {'key' => 'value'})
                                                               ^
spec/defines/resource_vhost_spec.rb:957:1: C: Style/EmptyLinesAroundBlockBody: Extra empty line detected at block body end. 

You don't need to test all the things from the example in location, but I'd test for absence when unset, and then test for presence of multiple variables, similar to what's done in the location example.

Also, you'll need to validate the parameter, right before:
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/resource/vhost.pp#L443-L445

  if ($fastcgi_param != undef) {
    validate_hash($fastcgi_param)
  }

[zachfi]

Alright, thank you for the information. I just run bundle exec rake spec before pushing, not the rest of the rake tasks as you have shown here. I'll come back to this.

[wyardley]

You need to check for vhost, not location, and I think you'll be looking at the template fragments.

[wyardley]

You're also not actually putting the parameters into any templates, and you're testing the wrong thing (nginx__resource__location).

Location does it a bit differently than we'd want to do it for vhost, see:

puppet-nginx/manifests/resource/location.pp

Lines 374 to 391 in f298908

	# Use proxy or fastcgi template if $proxy is defined, otherwise use directory template.
	if ($proxy != undef) {
	$content_real = template('nginx/vhost/locations/proxy.erb')
	} elsif ($location_alias != undef) {
	$content_real = template('nginx/vhost/locations/alias.erb')
	} elsif ($stub_status != undef) {
	$content_real = template('nginx/vhost/locations/stub_status.erb')
	} elsif ($fastcgi != undef) {
	$content_real = template('nginx/vhost/locations/fastcgi.erb')
	} elsif ($uwsgi != undef) {
	$content_real = template('nginx/vhost/locations/uwsgi.erb')
	} elsif ($www_root != undef) {
	$content_real = template('nginx/vhost/locations/directory.erb')
	} elsif ($try_files != undef) {
	$content_real = template('nginx/vhost/locations/try_files.erb')
	} else {
	$content_real = template('nginx/vhost/locations/empty.erb')
	}

puppet-nginx/templates/vhost/locations/fastcgi.erb

Lines 17 to 21 in f298908

	<% if defined? @fastcgi_param -%>
	<%- field_width = @fastcgi_param.inject(0) { |l,(k,v)| k.size > l ? k.size : l } -%>
	<%- @fastcgi_param.sort_by {|k,v| k}.each do |key, val| -%>
	fastcgi_param <%= sprintf("%-*s", field_width, key) %> <%= val %>;
	<%- end -%>

The location manifest needs to change, but currently, it will use one of several templates, which causes some other problems. For vhost I think, you'll probably want to put in both of:

https://github.com/voxpupuli/puppet-nginx/blob/master/templates/vhost/vhost_footer.erb#L5
https://github.com/voxpupuli/puppet-nginx/blob/master/templates/vhost/vhost_ssl_footer.erb#L5
(which means you will probably also need to test both ssl and non-ssl contexts)

[zachfi]

I'm building 2.3.1 locally for testing rubocop, so I'll sort that out in a moment.

For the testing you mention, I don't believe I'm testing the wrong thing. Specifically what I'm testing, is that when you specify the $fastcgi_param parameter on the nginx::resource::vhost resource, that it gets passed to the nginx::resource::location resource as is, unmodified. The tests for nginx::resource::location should handle everything that is necessary to validate this input, including the template content, but this PR doesn't touch any of that as its not a change captured in this PR.

That said, Is there a different change I should be making? I'm under the assumption here that the location created by default for every vhost when use_default_location is true is where the params for the fastcgi should go, as looks to be the case with most of the other params when using use_default_location. In which case, we just punt this param down the line to the location resource and do nothing if users disable lthe use_default_location param.

[zachfi]

I've rubocop'd like a pro. Hooray!

[zachfi]

I still want to make sure this is the right change to make, so if my thinking above is off, please correct me.

[wyardley]

I see what it's doing now and how (with the default location). I did find it a little confusing that it's in a different scope, but in any event, this is merged, and does behave the same way some similar / related params do. Thanks for fixing the Rubocop warnings.

[zachfi]

Great, thank you @wyardley