-
Notifications
You must be signed in to change notification settings - Fork 96
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
Support log level setting per plugin #268
base: master
Are you sure you want to change the base?
Support log level setting per plugin #268
Conversation
We must use `$log` for logging while configuring.
@qingling128 what does it take to resolve this PR? |
Looks like it needs rebasing. I tried the Github UI tool, but there is still a test failure. @okkez - Mind rebasing this off the latest master if it's still needed? |
Rubocop causes CI failure. I've fixed it. |
@@ -1132,15 +1130,15 @@ def set_vm_id | |||
@vm_id ||= fetch_gce_metadata('instance/id') if @platform == Platform::GCE | |||
@vm_id ||= ec2_metadata['instanceId'] if @platform == Platform::EC2 | |||
rescue StandardError => e | |||
@log.error 'Failed to obtain vm_id: ', error: e | |||
log.error 'Failed to obtain vm_id: ', error: e |
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.
Curious: How did we decide which cases are log
, and which cases are $log
?
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.
It's hidden in the extra part of the commit message on 63387b5. Frankly, I'd prefer it as a code comment in configure
.
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.
@okkez, can you please expand on what exactly this PR accomplishes? Frankly, I don't see how the commits relate to the PR title. Is there a real-world problem this solves? Is there a best practices document we can look at to justify these changes?
@@ -1132,15 +1130,15 @@ def set_vm_id | |||
@vm_id ||= fetch_gce_metadata('instance/id') if @platform == Platform::GCE | |||
@vm_id ||= ec2_metadata['instanceId'] if @platform == Platform::EC2 | |||
rescue StandardError => e | |||
@log.error 'Failed to obtain vm_id: ', error: e | |||
log.error 'Failed to obtain vm_id: ', error: e |
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.
It's hidden in the extra part of the commit message on 63387b5. Frankly, I'd prefer it as a code comment in configure
.
AFAIK, there is no document about |
No description provided.