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

enhance logging support per location: adding access_log & log_not_found directives #1151

Closed
wants to merge 16 commits into from

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,
As I needed to use the access_log and/or log_not_found for some locations on my project I've added it to the module.

Hope it helps
Best regards
François.

@wyardley
Copy link
Collaborator

While adding this directive can already be added via, e.g., location_cfg_append, as shown in the examples, I don't have a problem with the idea of this PR.
But:

  • I believe that access_log supports paths in locations as well, no? So in this case, the parameter should probably use the same style and allow the same types of options that are allowed for location. We're already somewhat inconsistent with how access log directives are handled in the module (between main config and server), but we shouldn't make the problem worse.
  • Tests should probably be added for this change
    http://nginx.org/en/docs/http/ngx_http_log_module.html

Copy link
Collaborator

@wyardley wyardley left a comment

Choose a reason for hiding this comment

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

Thanks for the contributions!
See my comments in review and in PR itself.
I think format_log and error_log are also supported?

@@ -212,6 +214,8 @@
Boolean $mp4 = false,
Boolean $flv = false,
Optional[String] $expires = undef,
Optional[Enum['on','off']] $log_not_found = undef,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@wyardley
Copy link
Collaborator

Thanks for the contribution! If you need help in terms of adding tests, squashing commits, or have other questions, pop in to #voxpupuli on Freenode IRC or on the Puppet Community Slack.

…ion: 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
@ceonizm
Copy link
Contributor Author

ceonizm commented Nov 21, 2017

Requested changes have been done.
Since I didn't see a valid reason to put multiple access_log directives in the same location I choose to let it as Optional[String] unlike the server.pp declaration.

@wyardley
Copy link
Collaborator

@ceonizm Not sure how easy this will be to fix now, but you need to rebase rather than merge.

Why would you not have a valid reason to have multiple access_log directives for a location? One example would be if you're logging to both a local filesystem and to syslog.

@ceonizm
Copy link
Contributor Author

ceonizm commented Nov 21, 2017

One example would be if you're logging to both a local filesystem and to syslog.
@wyardley ok it sounds like a valid reason :) (a bit overkill to my mind in the context of a location but definitely valid).
It should be done now.

@wyardley
Copy link
Collaborator

@ceonizm Looks like there's a conflict with something that got merged; can you rebase and squash?

@ceonizm
Copy link
Contributor Author

ceonizm commented Nov 24, 2017

@wyardley it should be done (at least I hope so)

@bastelfreak
Copy link
Member

Hi @ceonizm, please take a look at the git history. There are a lot of commits, and I can't cleanly rebase them. Can you please squash them into one commit? Feel freee to reach out to us on IRC if you need any help with git. We're in #voxpupuli on freenode.

@bastelfreak
Copy link
Member

Hi people. I'm going to close this PR due to inactivity. Please reopen and rebase if you're still interested in it.

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

Successfully merging this pull request may close these issues.

3 participants