-
Notifications
You must be signed in to change notification settings - Fork 183
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
Prevent "undefined local variable or method" error from enhance_k8s_metadata Fluent plugin #1212
Prevent "undefined local variable or method" error from enhance_k8s_metadata Fluent plugin #1212
Conversation
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.
@astencel-sumo You're truly a ruby master 🚀 🎉
@@ -17,7 +17,7 @@ def teardown | |||
end | |||
|
|||
def log | |||
Fluent::Test::TestLogger.new | |||
@test_log ||= Fluent::Test::TestLogger.new |
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.
Why this?
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.
If you look at the definition of log
method before the change, it was returning a new instance of TestLogger
on every call. This way, if you logged something with log.error(...)
somewhere in the code, and wanted to see all the logs with log.logs
later, you would always get an empty collection from log.logs
because, well, you were calling it on a brand new instance.
What the changed version does, it assigns a new instance of TestLogger
to class instance member @test_log
but only if @test_log
is empty - this is effectively a lazy instantiator for @test_log
and the log
method acts as the accessor for this member. This means that the first call to log
method will create a new TestLogger
instance and assign it to @test_log
, and all following calls to log
method will retrieve the TestLogger
instance created in the first call to the method.
assert_not_nil metadata | ||
assert_equal '1691804714', metadata['pod_labels']['pod_labels']['pod-template-hash'] | ||
assert_empty metadata['owners'] | ||
assert log.logs.any? { |log| log.include?('failed to fetch resource') }, "'failed to fetch resource' not found in logs: #{log.logs}" |
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.
can we test for full message here?
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 we can, I considered this but wanted to make it as concise as possible
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 think we should test it. It can be another test
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.
Please see change in 84b9cfb - is this what you had in mind @sumo-drosiek?
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, thanks
Description
This PR fixes the "undefined local variable or method `type'" error coming from enhance_k8s_metadata Fluent plugin.
Here's a sample error log:
Testing performed