MODULES-1382: support multiple access log directives #951
Conversation
|
Feedback on this implementation would be great! |
|
@jlambert121 i like the general look and feel of the design: the hash (struct, really ;) makes the intent very obvious. |
| <% elsif @access_log -%> | ||
| CustomLog "<%= @access_log_destination %>" <%= @_access_log_format %> | ||
| <% @_access_logs.each do |log| -%> | ||
| <% log['env'] ? env = "env=#{log['env']}" : env = '' -%> |
There was a problem hiding this comment.
this line can be simplified as:
env ||= "env=#{log['env']}" if log['env']There was a problem hiding this comment.
This being easier to read/simpler is a matter of opinion, I prefer ternary operators because I think they're easier to read. That being said, I don't mind changing it. I think later on in the template though <%= env %> could result to the literal 'nil' or 'undef' for older versions of ruby if log['env'](and therefore env) was unset, wouldn't it? The purpose of setting it to if log['env'] was undefined was to ensure the template was clean on output.
There was a problem hiding this comment.
i don't think so, but maybe that's why we need more tests ;)
There was a problem hiding this comment.
Actually:
env ||= "env=#{log['env']}" if log['env']
env ||= ''That said, I'd say this is not logic that should be in a template but should be passed and used as-is with the logic whereever the template is included at.
jlambert121
force-pushed
the
modules_1382
branch
3 times, most recently
from
ebce295 to
80c5f68
Compare
jlambert121
force-pushed
the
modules_1382
branch
from
80c5f68 to
bb96180
Compare
MODULES-1382: support multiple access log directives
|
thank you @jlambert121! |
MODULES-1382: https://tickets.puppetlabs.com/browse/MODULES-1382