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

Remove Sidekiq::Util #382

Merged

Conversation

mihaic195
Copy link
Contributor

Fixes #381

@marcelolx
Copy link
Member

I tested the changes locally and it seems that this does the trick! Thank you!

@marcelolx marcelolx merged commit 2fccd20 into sidekiq-scheduler:master Jun 8, 2022
@ltello
Copy link

ltello commented Jun 8, 2022

sidekiq/util has been replaced with sidekiq/component in sidekiq 6.5.0.
Why not make the replacement in this gem instead of removing it? Is sidekiq/util not needed by this gem at all? @mihaic195 @marcelolx

I did the replacement locally and got all this gem tests green

@marcelolx
Copy link
Member

@ltello The problem with that is that Sidekiq::Component does only exist in Sidekiq 6.5 which would force us to bump a major release of sidekiq-scheduler and this major release would require Sidekiq >= 6.5

@marcelolx
Copy link
Member

Also, I believe that sidekiq/util is not needed at all by this gem, the only method we were using was the logger which we can access without requiring this internal API (maybe in older versions of sidekiq it was required, but doesn't seem to be anymore)

@ltello
Copy link

ltello commented Jun 8, 2022

Thanks @marcelolx. Yes, logger seems to be the only dependency. So fine going with the first PR @mihaic195 pushed. Thanks both for fixing this.

@marcelolx
Copy link
Member

Another important thing to note is that we have been accessing the logger through Sidekiq.logger, including sidekiq/util would have allowed us to access it directly logger which we don't do, so I don't see reasons for requiring that file.

Looking at the history, it seems that since #238 we didn't depend anymore on any method of sidekiq/util, before we did access logger directly https://github.com/moove-it/sidekiq-scheduler/pull/238/files#diff-0ddd2bcc9bd29aac261acdd76530920c85887c269cc29e3f054e465b4ac8699eL76 but with that refactor we don't do anymore

@tvdeyen
Copy link

tvdeyen commented Jun 9, 2022

This was quick! Thank you 🙏🏻

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.

Sidekiq 6.5.0 upgrade is breaking this gem
4 participants