Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adding support for include directive in map #1149

Merged
merged 3 commits into from
Nov 18, 2017

Conversation

ceonizm
Copy link
Contributor

@ceonizm ceonizm commented Nov 16, 2017

As requested in previous PR (#1145) I've splitted features in multiples branches and PR:

Hello,
I needed to use the include directive in a map block to handle some redirections that were dynamically generated by an other app. I noticed this feature was missing from your module so I've added it.

Best Regards
François.

@@ -67,6 +76,7 @@
Variant[Array, Hash] $mappings,
Optional[String] $default = undef,
Enum['absent', 'present'] $ensure = 'present',
Array[String] $includes = [],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work if it's an empty array, since you're not checking if it's empty in the template? Will this create an empty includes line?
Maybe this should be Optional[Array[String]] $includes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because there's a default value you can omit it ; it acts as an optional parameter.
So if you pass an empty Array it will be the same as passing nothing and use the default value: it won't iterate nothing into the loop in template so nothing will be written (no empty includes line).
I agree with you since it is finally an optional parameter that declaring it explicitly as Optional would be better (but in that case the test in the template will became mandatory).

@wyardley
Copy link
Collaborator

Can you add spec tests for this?
And is the case where the array is empty safe? Maybe this should be optional (see review comment)?

@wyardley
Copy link
Collaborator

FWIW, there's a consistency issue here as well, as server resource uses include_files:
https://github.com/voxpupuli/puppet-nginx/blob/master/manifests/resource/server.pp#L100
https://github.com/voxpupuli/puppet-nginx/blob/master/templates/server/server_footer.erb#L1-L4
Personally, I actually like include better, since it matches the directive, as long as there's no conflict, but IMHO, it's best if server and location use like parameters for like directives, if that makes sense.

@alexjfisher
Copy link
Member

I much prefer not making it Optional and defaulting to the empty array instead. As @ceonizm points out, there will be no output in the template when the array is empty.

For consistency with server.pp I agree the parameter name should be include_files (even if includes would have otherwise been a better name).

I'd be tempted to change the server.pp include_files to also just default to an empty array and simplify the template.

<% if @include_files -%>
and could then be safely removed.

root added 2 commits November 18, 2017 10:36
… with the other manifests

adding test to ensure empty value , single and multiple inclusions works correctly
@wyardley wyardley merged commit 2ea5aac into voxpupuli:master Nov 18, 2017
@wyardley
Copy link
Collaborator

I squash-merged, thanks for the contribution @ceonizm! With the other PRs, if you're able to squash / force-push (or break into logical commits), that would be great.

cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
* adding support for include directive in map

* change param name includes to include_files to keep naming consistent with the other manifests
adding test to ensure empty value , single and multiple inclusions works correctly

* fix rubocop offenses
ceonizm added a commit to ceonizm/puppet-nginx that referenced this pull request Feb 7, 2020
* adding support for include directive in map

* change param name includes to include_files to keep naming consistent with the other manifests
adding test to ensure empty value , single and multiple inclusions works correctly

* fix rubocop offenses
ceonizm pushed a commit to ceonizm/puppet-nginx that referenced this pull request Feb 7, 2020
adding support for include directive in map (voxpupuli#1149)

* adding support for include directive in map

* change param name includes to include_files to keep naming consistent with the other manifests
adding test to ensure empty value , single and multiple inclusions works correctly

* fix rubocop offenses

adding support for proxy_cache_bypass and proxy_cache_lock (voxpupuli#1150)

* adding support for proxy_cache_bypass and proxy_cache_lock

* adding tests and restrict proxy_cache_lock to enum

	modifié :         manifests/resource/location.pp
	modifié :         manifests/resource/map.pp
	modifié :         manifests/resource/server.pp
	modifié :         spec/defines/resource_location_spec.rb
	modifié :         spec/defines/resource_map_spec.rb
	modifié :         templates/conf.d/map.erb
	modifié :         templates/server/locations/proxy.erb
ceonizm added a commit to ceonizm/puppet-nginx that referenced this pull request Feb 7, 2020
…nd directives

adding documentation comments

adding support for include directive in map (voxpupuli#1149)

* adding support for include directive in map

* change param name includes to include_files to keep naming consistent with the other manifests
adding test to ensure empty value , single and multiple inclusions works correctly

* fix rubocop offenses

adding support for proxy_cache_bypass and proxy_cache_lock (voxpupuli#1150)

* adding support for proxy_cache_bypass and proxy_cache_lock

* adding tests and restrict proxy_cache_lock to enum

fixing access_log, adding error_log and adding tests

access_log: adding format_log to stay consistent with server declaration: when using it just disable logging, error_log: adding error_level to be able to set a different error level per location if needed); passing off disable the error_log

changing type to Optional[Variant[Array, String]] for access_log & error_log to handle multiple declaration

changing type to Optional[Variant[Array, String]] for access_log & error_log to handle multiple declaration
ceonizm added a commit to ceonizm/puppet-nginx that referenced this pull request Mar 10, 2020
adding support for include directive in map (voxpupuli#1149)

* adding support for include directive in map

* change param name includes to include_files to keep naming consistent with the other manifests
adding test to ensure empty value , single and multiple inclusions works correctly

* fix rubocop offenses

(cherry picked from commit 2ea5aac)
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
* adding support for include directive in map

* change param name includes to include_files to keep naming consistent with the other manifests
adding test to ensure empty value , single and multiple inclusions works correctly

* fix rubocop offenses
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants