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.

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

Only modify logger if it's an instance of ::Logger. #5674

Closed
wants to merge 1 commit into from

Conversation

ioquatix
Copy link

@ioquatix ioquatix commented Nov 30, 2022

#5673

At least don't assume that the logger argument is compatible with extend(Sidekiq::LoggingUtils).

@ioquatix ioquatix marked this pull request as ready for review November 30, 2022 09:52
@mperham
Copy link
Collaborator

mperham commented Nov 30, 2022

8e3f8f5

@mperham mperham closed this Nov 30, 2022
@mperham
Copy link
Collaborator

mperham commented Nov 30, 2022

With this change, custom loggers will just silently lose the ability for job-specific log levels. The test shows how to extend S::LoggingUtils in order to restore this functionality.

@ioquatix
Copy link
Author

My PR didn't break this use case for cases where it worked, but still mutated the incoming logger argument. While I think 8e3f8f5 is the best approach, it's also the least compatible. The compatibility is probably unlikely to be missed, as the default logger retains this functionality, so it only affects people deliberately assigning to Sidekiq::Config#logger=.

@ioquatix ioquatix deleted the safe-logger-assignment branch November 30, 2022 18:53
@ioquatix
Copy link
Author

@mperham is there any chance to cut a release with this change? As we are blocked on it.

@mperham
Copy link
Collaborator

mperham commented Nov 30, 2022

I've got a number of other things in the fire right now but I hope to release 7.0.2 this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants