-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[chef/chef]Fix duplicate logs #7698
[chef/chef]Fix duplicate logs #7698
Conversation
41ed097
to
f45e775
Compare
Since we are no longer checking to see if the session is a TTY I am curious to know what happens when this is invoked over a remote non-interactive SSH session, or when it is invoked via a service such as runit, cron, systemd, or Windows scheduled task. |
Hi @rhass , Reference for logic to not using TTY is #7491 (comment) I've manually tested using
I am not sure if this require test cases to be written since test cases for setting logger is already there. |
@dheerajd-msys: |
lib/chef/config.rb
Outdated
@@ -80,5 +80,8 @@ class Config | |||
end | |||
end | |||
|
|||
def self.is_default?(key) | |||
key.is_a?(IO) | |||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no you still can't override the meaning of this.
lib/chef/application.rb
Outdated
@@ -205,7 +205,7 @@ def configure_log_location | |||
# Based on config and whether or not STDOUT is a tty, should we setup a | |||
# secondary logger for stdout? | |||
def want_additional_logger? | |||
Chef::Config.is_default?(:log_location) && Chef::Config[:log_location].tty? && !Chef::Config[:daemonize] | |||
!(Chef::Config.is_default?(Chef::Config[:log_location]) || STDOUT.closed? || Chef::Config[:daemonize]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing the boolean logic here is also something that i'm pretty sure i investigated already and doesn't work correctly in the other use case, pretty certain that means you've changed the meaning of this check.
Chef::Config[:log_location].class == IO && Chef::Config[:log_location].tty? && !Chef::Config[:daemonize]
might work correctly in both use cases and not break anything important.
If we have both an actual IO which is a tty then its very likely the log_location is STDOUT.
We want to be very careful here since the :log_location may be a String pathname or a logger instance, we might also support File objects (IDK, but I wouldn't be surprised since those will inherit from IO and would have always worked -- since they probably ducktype and work someone may have decided to use one). By limiting the check to only the EXACT class match on IO we will still support overriding this with a File. This check will now only have an edge condition that if someone hands us an actual IO object that isn't an instance of a subclass this check would fail inappropriately (but the tty? check hopefully catches that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @lamont-granquist for review and suggestions, It gave me an idea to not override the is_default?
instead trying out to change the code as per your suggestion.
Although the code Chef::Config[:log_location].class == IO && Chef::Config[:log_location].tty? && !Chef::Config[:daemonize]
didn't seem to work out and we still get the duplicate loggers for STDOUT
.
But if we change it to !(Chef::Config[:log_location].class == IO || STDOUT.closed? || Chef::Config[:daemonize])
then this worked out well for STDOUT
and path/to/file
and doesn't require to override is_default?
as well.
Tested it with chef-client
, chef-solo
and chef-apply
.
Please let me know your thoughts about this and we can then push the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does ( Chef::Config[:log_location].class != IO ) && STDOUT.tty? && !Chef::Config[:daemonize]
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @lamont-granquist, I've tried with this code change as well and it is working fine.
( Chef::Config[:log_location].class != IO ) && STDOUT.tty? && !Chef::Config[:daemonize]
works out well with chef-client
, chef-apply
& chef-solo
for STDOUT
and /path/to/file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is the one to go with then. it should preserve the old code behavior. the way it used to look in v12.0.0 was:
( Chef::Config[:log_location] != STDOUT ) && STDOUT.tty? && (!Chef::Config[:daemonize]) && (Chef::Config[:force_logger])
the force_logger bit is no longer relevant. we've now preserved the first three conditional -- only the first conditional has been changed to != on the class being IO rather than != on the instance being STDOUT.
i think i'm good with that.
Signed-off-by: dheerajd-msys <[email protected]>
1c4fb58
to
4ab993f
Compare
@lamont-granquist Please take a look. |
Signed-off-by: dheerajd-msys <[email protected]>
4ab993f
to
e8fa4f1
Compare
merged! |
@lamont-granquist: excuse my lame question, when a new chef version\gem will be released? |
@y0y0z We're aiming for later this week |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Signed-off-by: dheerajd-msys [email protected]
Description
This fixes the duplicate logs for
log_location
Issues Resolved
fixes #7184
Manual testing completed :
a) when :log_location is set STDOUT
b) when :log_location is set to a file "/path/to/logfile"
a) when :log_location is set STDOUT
b) when :log_location is set to a file "/path/to/logfile"
a) when :log_location is set STDOUT
b) when :log_location is set to a file "/path/to/logfile"
Check List