-
Notifications
You must be signed in to change notification settings - Fork 18
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
Fix Logger subclasses providing no default logdev #86
Fix Logger subclasses providing no default logdev #86
Conversation
Logger.new requires a log device as the first argument. Normalized the other loggers to match the Logger#initialize interface for consistency. Fixes the following bug: irb(main):001> ManageIQ::Loggers::Base.new xxx/.rubies/ruby-3.3.6/lib/ruby/3.3.0/logger.rb:578:in `initialize': wrong number of arguments (given 0, expected 1..3) (ArgumentError) from manageiq-loggers (1.2.0) lib/manageiq/loggers/base.rb:27:in `initialize' irb(main):002> ManageIQ::Loggers::JSONLogger.new xxx/.rubies/ruby-3.3.6/lib/ruby/3.3.0/logger.rb:578:in `initialize': wrong number of arguments (given 0, expected 1..3) (ArgumentError) from manageiq-loggers (1.2.0) lib/manageiq/loggers/base.rb:27:in `initialize' from manageiq-loggers (1.2.0) lib/manageiq/loggers/json_logger.rb:5:in `initialize'
def initialize(*_, **_) | ||
def initialize(_logdev = nil, *_, **_) |
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.
I'm not sure I understand how this is different than what was there before. Previously we just pass along what was there. Is this specific to a particular Ruby version?
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.
Logger initialize requires 1+ arguments. Nothing is providing a logdev default previously for Base or JSONLogger.
def initialize(logdev, shift_age = 0, shift_size = 1048576, level: DEBUG,
progname: nil, formatter: nil, datetime_format: nil,
binmode: false, shift_period_suffix: '%Y%m%d')
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.
I figured it was better to match the interface as super
with a default than to explicitly pass it in the super call.
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.
Yeah I guess you could argue that you should be passing in something for the first arg since Logger.new also doesn't have a default value (Logger.new fails with the same error)
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.
Yeah what @agrare said is what I was getting at
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.
[1] pry(main)> require "logger"
[2] pry(main)> Logger.new
~/.gem/ruby/3.3.6/gems/logger-1.6.5/lib/logger.rb:581:in `initialize': wrong number of arguments (given 0, expected 1..3) (ArgumentError)
from (pry):2:in `new'
from (pry):2:in `__pry__'
from <internal:kernel>:187:in `loop'
[3] pry(main)> Logger.new(nil)
#<Logger:0x0000000133d91708 @default_formatter=#<Logger::Formatter:0x0000000133d90f88 @datetime_format=nil>, @formatter=nil, @level=0, @level_override={}, @logdev=nil, @progname=nil>
We're intentionally trying to match the Logger gem's interface.
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.
I haven't figured out the automation engine failures in 7.1 that raised this error. I'll reopen if I figure it out. It seemed odd we provided a default logdev for containers and journald loggers but not any of the others.
So nothing used these base classes directly before, only Journald and Container which both handled the _logdev argument. 👍 lgtm |
I somehow hit this in rails 7.1 test failures in automation engine when it was trying to stub container loggers. 🤷 |
Maybe ActiveSupport changes something by patching logger (or perhaps more_core_extensions)? |
ActiveSupport::Logger fails in the same way (as expected) > ActiveSupport::Logger.new
~/.gem/ruby/3.3.6/gems/logger-1.6.5/lib/logger.rb:581:in `initialize': wrong number of arguments (given 0, expected 1..3) (ArgumentError) |
Logger.new requires a log device as the first argument.
Normalized the other loggers to match the Logger#initialize interface for consistency.
Fixes the following bug: