Skip to content

[WIP] Move resource id param to logger initialize #570

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

agrare
Copy link
Member

@agrare agrare commented Apr 4, 2025

No description provided.

@agrare agrare force-pushed the move_resource_id_param_to_logger_initialize branch 2 times, most recently from 1a57f86 to 3666834 Compare April 4, 2025 13:33
@agrare agrare force-pushed the move_resource_id_param_to_logger_initialize branch from 3666834 to d1a9849 Compare April 4, 2025 15:30
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

good stuff

automation_log_wrapper.add(severity, message, progname, &block)

severity ||= Logger::UNKNOWN
return true if severity < @level
Copy link
Member

Choose a reason for hiding this comment

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

Do we want our level, or the level of our wrapped log?

(that is the whole automation_log_wrapper.send("#{severity}?") thing)

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we sync the :level from the wrapped logger when we build it initially? I don't want to add more call-outs to the 'hard-coded' log_wrapper since I want to try to drop this and just use a normal broadcast

private

def add(severity, message = nil, progname = nil, &block)
automation_log_wrapper.add(severity, message, progname, &block)
Copy link
Member

Choose a reason for hiding this comment

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

think this wants to be after we resolve message?

Doesn't seem optimal if we message ||= yield and then have automation_log_wrapper have to do the same thing

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking of the wrapped logger had a different progname that it wanted to use this would allow that

Copy link
Member Author

Choose a reason for hiding this comment

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

Plus I think this matches the existing behavior because it used to call the wrapped logger directly from def info as opposed to from add

@kbrock kbrock self-assigned this Apr 9, 2025
@miq-bot miq-bot added the wip label Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants