Allow multiple servers per location #1278
Conversation
SaschaDoering
force-pushed
the
1187_recycle_locations
branch
2 times, most recently
from
0c99478
to
88cba1d
Compare
| content => template('nginx/server/location.erb'), | ||
| order => $priority, | ||
| } | ||
| any2array($server).each |$s| { |
There was a problem hiding this comment.
From our docs:
If you can supply one or multiple values for an attribute it’s common practice to enforce the datatype for one value and an array of that datatype. An example for string is Variant[String[1],Array[String[1]]]. This can be used in the Puppet code as [$var].flatten()
Maybe we should update the docs to any2array(). I'm not sure if this is easier to read.
manifests/resource/location.pp
Outdated
| if ($ssl == true or $ssl_only == true) { | ||
| $ssl_priority = $priority + 300 | ||
| $config_file = "${server_dir}/${server_sanitized}.conf" | ||
| if $ensure == present { |
There was a problem hiding this comment.
Please add quotes:
| if $ensure == present { | |
| if $ensure == 'present' { |
There was a problem hiding this comment.
Hello, you're right. I add qoutes to present.
bastelfreak
added
enhancement
|
Hi @SaschaDoering, thanks for the PR. Can you please add an acceptance test for this as well? Otherwise it looks good! |
SaschaDoering
force-pushed
the
1187_recycle_locations
branch
3 times, most recently
from
0fc9114
to
8265885
Compare
|
Hi, I add an acceptance test for locations. This is my first one so I hope it's allright. Also I fixed the issue with the missing quotes. I'm not sure how to understand your comment for updating the docs with |
|
Sorry about the confusion, that was basically a note for myself :D |
|
This looks good to me, but I will keep it open for a few days in case somebody else wants to review it as well. |
manifests/resource/location.pp
Outdated
| @@ -123,6 +122,14 @@ | |||
| # server => 'test2.local', | |||
| # } | |||
| # | |||
| # Use one location in multiple serrvers | |||
There was a problem hiding this comment.
Sorry about the confusion, that was basically a note for myself :D
Not for that, thank you for the review ;)
There was a problem hiding this comment.
Thx for the finding, I've fixed that.
manifests/resource/location.pp
Outdated
| @@ -7,8 +7,7 @@ | |||
| # (present|absent) | |||
| # [*internal*] - Indicates whether or not this location can be | |||
| # used for internal requests only. Default: false | |||
| # [*server*] - Defines the default server for this location | |||
| # entry to include with | |||
| # [*server*] - Defines all server for this location entry to include with | |||
There was a problem hiding this comment.
This documentation doesn't read very well. Could you try rephrasing?
There was a problem hiding this comment.
What about 'Defines a server or list of servers that integrate these location'?
There was a problem hiding this comment.
I changed the text, hopefully it's better than the last try :)
By changing the data type, you can now specify multiple servers for a location. Fixes voxpupuliGH-1187
SaschaDoering
force-pushed
the
1187_recycle_locations
branch
from
8265885
to
117b156
Compare
Allow multiple servers per location
Allow multiple servers per location
Pull Request (PR) description
By changing the data type, you can now specify multiple servers for
a location. These are then rolled out in a loop. This doesn't break existing installations.
This Pull Request (PR) fixes the following issues
Fixes GH-1187