Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add a wrap method to support BroadcastLogger/ActiveSupport::Logger.broadcast #63

Merged
merged 1 commit into from
Sep 30, 2024
Merged

Add a wrap method to support BroadcastLogger/ActiveSupport::Logger.broadcast #63

merged 1 commit into from
Sep 30, 2024

Conversation

agrare
Copy link
Member

@agrare agrare commented Dec 13, 2023 •
edited
Loading

Rails has removed support for ActiveSupport::Logger.broadcast in the same release they added a ActiveSupport::BroadcastLogger. This means in order to support both active_support < 7.1 and active_support >= 7.1 we need to special case this based on the gem version

TODO:

  • journald logger caller_locations is going to be different if called directly, from BroadcastLogger, or from Loggers.broadcast

@Fryguy Fryguy added the wip label Jan 17, 2024
@agrare agrare force-pushed the support_broadcast_logger branch 3 times, most recently from 118736c to 324068a Compare September 26, 2024 17:11
@Fryguy Fryguy mentioned this pull request Sep 26, 2024
Merged
@Fryguy
Copy link
Member

Fryguy commented Sep 26, 2024

@agrare I opened #77 to add in the matrix testing of ActiveSupport - once that's merged you can rebase on that, and add rails 7.1 in the matrix in this PR.

@agrare agrare force-pushed the support_broadcast_logger branch 2 times, most recently from 05882cb to 0e75702 Compare September 26, 2024 21:33
@kbrock
Copy link
Member

kbrock commented Sep 26, 2024

kicking

@kbrock kbrock closed this Sep 26, 2024
@kbrock kbrock reopened this Sep 26, 2024
@agrare
Copy link
Member Author

agrare commented Sep 26, 2024

@kbrock still WIP and not green locally yet

@agrare agrare force-pushed the support_broadcast_logger branch 3 times, most recently from 3c2aad5 to 4312a1a Compare September 27, 2024 13:38
@agrare agrare changed the title [WIP] Add a wrap method to support BroadcastLogger/ActiveSupport::Logger.broadcast Add a wrap method to support BroadcastLogger/ActiveSupport::Logger.broadcast Sep 27, 2024
@agrare
Copy link
Member Author

agrare commented Sep 27, 2024

Okay got this green across 6.1..7.1, taking out of WIP

@Fryguy Fryguy added enhancement New feature or request and removed wip labels Sep 27, 2024
@Fryguy Fryguy self-assigned this Sep 27, 2024
@agrare agrare mentioned this pull request Sep 30, 2024
Closed
@agrare agrare force-pushed the support_broadcast_logger branch from 4312a1a to f8bf711 Compare September 30, 2024 16:16
jrafanie
jrafanie approved these changes Sep 30, 2024
View reviewed changes
Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM, followup in #79 looks to address the concern about assuming container logger is what is desired.

@jrafanie jrafanie merged commit f11de9c into ManageIQ:master Sep 30, 2024
16 checks passed
@jrafanie jrafanie assigned jrafanie and unassigned Fryguy Sep 30, 2024
@agrare agrare deleted the support_broadcast_logger branch September 30, 2024 16:57
This was referenced Sep 30, 2024
Open
Open
agrare added a commit that referenced this pull request Sep 30, 2024
@agrare
Release v1.2.0
1ed2572 
Added
- Test with ruby 3.1 and 3.0 (#65)
- Add ActiveSupport versions into the test matrix (#77)
- Add a wrap method to support BroadcastLogger (#63)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy left review comments

@jrafanie jrafanie jrafanie approved these changes

Assignees

@jrafanie jrafanie

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@agrare @Fryguy @kbrock @jrafanie