Add include files to locations

[cdenneen]

No description provided.

[interactiveblueprints]

It would be nice if this was pulled.
We often use include in location.

[3flex]

Two things before we can merge:

• Please add tests for the new functionality
• Please rename the new parameter to "include" to align with the nginx directive name. I know this will not be the same as the parameter on vhost but we'd like to make any future additions the same name as the nginx directive.

[cdenneen]

@3flex if you could point me in the right direction I'd be happy to help but I've never written a test and don't even know where to start.

[3flex]

@cdenneen take a look at the spec/defines/resource_location_spec.rb file. You would add the new tests around https://github.com/jfryman/puppet-nginx/blob/master/spec/defines/resource_location_spec.rb#L167, and can base them on https://github.com/jfryman/puppet-nginx/blob/master/spec/defines/resource_location_spec.rb#L90-L98 (another parameter that accepts arrays).

Essentially you're adding a test that says when "include_files" is set to a given array, you expect certain output (though please rename parameter to "include" as I mentioned above).

When you commit then push to this branch you'll get Travis to test the results so for now you won't have to get the tests running locally, though if you do you'll get much faster feedback. PuppetLabs has a getting started guide which will help: https://github.com/puppetlabs/puppetlabs-apache/blob/master/CONTRIBUTING.md#getting-started. You can ignore what it says about beaker.

[cdenneen]

@3flex so do you know if someone will be updating this to be include instead of include_files as well?
https://github.com/jfryman/puppet-nginx/blob/master/manifests/resource/vhost.pp#L121

[3flex]

That will come later - there are some refactorings and internal module changes that will be worked on in the next little while. One of the items is to make sure the parameter/directive names match up as much as possible between the module and nginx.

For right now though it will be left as is.

[cdenneen]

shouldn't we be using prepended values though? vhost_include, location_include, etc? otherwise what's to stop @include populating location include files in vhost includes?

[3flex]

Forgive my ignorance, I don't see how that would happen? According to Puppet docs

Code inside a class definition or defined type exists in a local scope.

So include should not populate anywhere outside the vhost/location it's defined in. Or did you mean something else?

[cdenneen]

true... ok... modifying the patch now...

[3flex]

Fantastic, thanks for making those changes and adding tests!

Last thing before we merge: can you please squash this down to a single commit. I promise I'll merge as soon as that's done!

[cdenneen]

@3flex done