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

Redirect accesslog issue 1403 #1528

Merged
merged 2 commits into from
Aug 22, 2017

Conversation

hramezani
Copy link
Contributor

No description provided.

@hramezani hramezani force-pushed the redirect_accesslog_issue_1403 branch from 62f09cd to d31b8ac Compare June 14, 2017 07:03
@tilgovi
Copy link
Collaborator

tilgovi commented Jul 12, 2017

If we do this we should probably have the default to be True so that it's not changing the defaults. What do you think?

@tilgovi
Copy link
Collaborator

tilgovi commented Jul 12, 2017

Also, I think it would be preferable if the option made it clear from its name that it's redirecting to syslog.

@tilgovi
Copy link
Collaborator

tilgovi commented Jul 12, 2017

Oh, I see now that the issue that motivated this. Maybe we should change the default. We could do that for R20. What do you think, @benoitc?

@hramezani
Copy link
Contributor Author

@benoitc @tilgovi, I am waiting for your decision for default value

@berkerpeksag
Copy link
Collaborator

I agree that we should keep the default behavior in 19.x. I agree with @tilgovi's plan:

In 19.x:

  • Add a --disable-access-log-redirection (we should probably include "syslog" to the option name) to disable current behavior.

In 20.0:

  • Change default behavior not to send access logs to syslog.
  • Add a new option to send access logs to syslog.
  • Remove --disable-access-log-redirection.

@hramezani hramezani force-pushed the redirect_accesslog_issue_1403 branch from d31b8ac to 450f702 Compare July 14, 2017 10:41
@hramezani
Copy link
Contributor Author

@berkerpeksag thanks, please check my changes

Copy link
Collaborator

@tilgovi tilgovi left a comment

Choose a reason for hiding this comment

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

LGTM

* ``False``

Disable redirect access logs to syslog.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs

.. versionadded:: 19.8

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, it would be nice to add a note to syslog documentation at http://docs.gunicorn.org/en/stable/settings.html#syslog

.. versionchanged:: 19.8

   You can now disable sending access logs by using the
   :ref:`disable-access-log-redirection` setting.

@@ -314,7 +315,8 @@ def access(self, resp, req, environ, request_time):
for format details
"""

if not (self.cfg.accesslog or self.cfg.logconfig or self.cfg.syslog):
if not (self.cfg.accesslog or self.cfg.logconfig or \
Copy link
Collaborator

Choose a reason for hiding this comment

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

\ is not needed here since we are already in if not (...).

@hramezani hramezani force-pushed the redirect_accesslog_issue_1403 branch 3 times, most recently from 78dcce7 to 5d72ad8 Compare July 15, 2017 05:57
@hramezani
Copy link
Contributor Author

@berkerpeksag thanks, requested changes are ready

@benoitc
Copy link
Owner

benoitc commented Aug 22, 2017

hrm the option name is misleading somehow (too much generic). maybe --disable-redirect-access-to-syslog or --redirect-access-to-syslog=no ? I quite like the second.

@benoitc benoitc mentioned this pull request Aug 22, 2017
@hramezani hramezani force-pushed the redirect_accesslog_issue_1403 branch from 5d72ad8 to 919871d Compare August 22, 2017 18:15
@hramezani
Copy link
Contributor Author

@benoitc, thanks, I rename the config with the first one --disable-redirect-access-to-syslog

Copy link
Collaborator

@tilgovi tilgovi left a comment

Choose a reason for hiding this comment

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

👍

@benoitc benoitc merged commit 60efb10 into benoitc:master Aug 22, 2017
@benoitc
Copy link
Owner

benoitc commented Aug 22, 2017

@hramezani thanks!

@tnir tnir mentioned this pull request Sep 18, 2017
mjjbell pushed a commit to mjjbell/gunicorn that referenced this pull request Mar 16, 2018
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.

4 participants