Sidekiq v7: fixes #152
Sidekiq v7: fixes #152
Conversation
jlestavel
force-pushed
the
sidekiq-v7-fixes
branch
from
9b59fb8 to
7bb88aa
Compare
jlestavel
force-pushed
the
sidekiq-v7-fixes
branch
from
7bb88aa to
a6ab02c
Compare
| if Sidekiq::LimitFetch.post_7? | ||
| options.config[:fetch] = Sidekiq::LimitFetch | ||
| capsule_or_options.config[:fetch_class] = Sidekiq::LimitFetch |
There was a problem hiding this comment.
If I'm not wrong, starting from Sidekiq v7, the option to define a custom "fetcher" is fetch_class, cf https://github.com/mperham/sidekiq/blob/4d3f90ba56dd9f9faa35a18bbdfb1b5cb726ee5c/lib/sidekiq/capsule.rb#L37
| @@ -22,6 +22,10 @@ def post_7? | |||
| @post_7 ||= Gem::Version.new(Sidekiq::VERSION) >= Gem::Version.new('7.0.0') | |||
| end | |||
|
|
|||
| def post_6_5? | |||
There was a problem hiding this comment.
This method is moved to have it public (to be able to use it from tests)
| if Sidekiq::LimitFetch.post_7? | ||
| options = options.config | ||
| end | ||
| def start(capsule_or_options) |
There was a problem hiding this comment.
This is a renaming to make it clear that the argument maybe a Capsule object or an options Hash, depending on the Sidekiq version
| @@ -0,0 +1,17 @@ | |||
| RSpec.describe Sidekiq::Manager do | |||
There was a problem hiding this comment.
I added this test to have a better coverage (because this would have failed when upgrading to Sidekiq v7)
|
@rgaufman you might be interested by this one |
|
I actually downgraded to sidekiq6 for now but thank you, this is great! |
|
Thanks! 👍 |
I think there are some missing changes to have the gem compatible with Sidekiq v7. The main one is the change on the customization of the "fetching class": it's now
fetch_classand notfetchanymore (cf my GH comment).I recommend to use the "Hide whitespace option" when reviewing, because there is some indentation change in
spec/sidekiq/limit_fetch/global/monitor_spec.rbfile.