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

Allow multiple access / error logs in main config and vhosts, other logging changes #888

Merged
merged 1 commit into from
Oct 6, 2016
Merged

Allow multiple access / error logs in main config and vhosts, other logging changes #888

merged 1 commit into from
Oct 6, 2016

Conversation

wyardley
Copy link
Collaborator

@wyardley wyardley commented Sep 28, 2016

This PR should preserve backwards compatibility, but allows to specify multiple access and error log directives by passing in an array, resulting in something like the following:

  access_log            /var/log/nginx/test.log combined;
  access_log            syslog:server=localhost combined;
  error_log             /var/log/nginx/test-error.log;
  error_log             syslog:server=localhost;

This allows more complex configurations, admittedly at the cost of doing more work in the template, something that it seems was intentionally avoided before. Note that the severity (for main error log) or access log format (in http or vhost scope) can't be set separately per logfile, though I think this could still be done if necessary using raw config appends (or I could change it to take a hash instead of an array when it's not a string).

Some other changes:

  • Support disabling the logs in vhost context by setting to the string 'absent'. This is different from setting them to 'off', since it should just default to the logfiles specified in nginx.conf. I am not sure that using 'absent' is the best way to do this, but because of the way the module works, doing undef doesn't seem practical (nor ideal when hiera is the data source); I'm open to suggestions for how to improve this (maybe just an empty string?)
  • I don't think the bit about having the format specified in nginx::vhost::access_log freeform actually worked as documented, since even before, it followed $format_log.
  • Adds the $http_format_log parameter in nginx::config (didn't add the passthrough for the deprecated way of setting since it's new). I left it out if it's set to default (undef); this will still give default ('combined') format, but will let you easily set 'off' there too if it's legal.

@petems
Copy link
Member

petems commented Sep 29, 2016

I think the cost of having a slightly more complex template is worth this functionality. The old case statement code wasn't super clean anyways.

This could probably be refactored into a function or some cleaner manifest code, but overall this is good. I'm going to run acceptance tests then merge 👍

@wyardley
Copy link
Collaborator Author

wyardley commented Sep 30, 2016

@petems @bastelfreak: This had a whole bunch of conflicts after the template changes. I resolved the conflicts in my branch. I did a weird merge to start with, but later squash-merged the whole thing, so hopefully should avoid someone having to squash merge it later.

I know this was approved already, but I'd appreciate if someone can give their take on what the best approach for the 'absent' thing in the first bullet point before this actually gets merged in. Happy to chat about it more tomorrow. I don't really like the "magic" of having 'absent' as a string in to suppress the line; would it make more sense to have an empty string do that?

I ran acceptance tests on one platform and the tests seem to be passing, but would be great if someone can also confirm that my git tomfoolery didn't mess anything up.

Also made a couple more minor cosmetic changes to fix spacing stuff in the templates (re: #891)

…g directives in vhost context, and allowing changing the access log format in the main scope
@wyardley
Copy link
Collaborator Author

wyardley commented Oct 6, 2016

@petems @bastelfreak: haven't heard any more feedback, but going to merge this myself, since it was approved before. Per discussion on Slack, so far, people seem Ok with the 'absent' (vs. undef) as a way to suppress the directive in a section.

Would love some help or feedback in terms of refactoring this to be cleaner (and, if we could make a more general function to take in a string, array, or hash, and return it as an array or hash, we could probably streamline the templates somewhat). If anyone wants to work on any of this at the contributor summit, I will be there.

@wyardley wyardley merged commit 1aad14a into voxpupuli:master Oct 6, 2016
@wyardley wyardley deleted the feature_log_behavior branch October 27, 2016 17:16
cegeka-jenkins pushed a commit to cegeka/puppet-nginx that referenced this pull request Sep 13, 2019
Allow multiple access / error logs in main config and vhosts, allow suppressing log directive in certain contexts, other logging changes
Rubueno pushed a commit to Rubueno/puppet-nginx that referenced this pull request Oct 19, 2020
Allow multiple access / error logs in main config and vhosts, allow suppressing log directive in certain contexts, other logging changes
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.

2 participants