ixti / sidekiq-throttled

Concurrency and rate-limit throttling for Sidekiq
MIT License
701 stars 75 forks source link

fix setup for sidekiq 6.1.0 #85

Closed hmaack closed 4 years ago

hmaack commented 4 years ago

https://github.com/mperham/sidekiq/compare/v6.0.7...v6.1.0 shows internal API changes. The setup needs to initialize the fetcher instead of passing the fetch class. 🤷‍♂️

https://github.com/mperham/sidekiq/issues/4614

I tried to fix that the easy way.

hmaack commented 4 years ago

fixed the rubocop issue but the CI has to run again 🕵️‍♂️

shlima commented 4 years ago

Waiting for this a lot

jobwat commented 4 years ago

is there any way to retrigger the CI? a empty commit?

schonert commented 4 years ago

@hmaack awesome!

Is there anything we can do to push this forward?

@jobwat it seems that it only runs once when the pull request is created. It would require a new PR. https://github.com/sensortower/sidekiq-throttled/blob/master/.github/workflows/ci.yml#L7

hmaack commented 4 years ago

The throttler gem supports versions below sidekiq 6.1, this PR is still not compatible for versions < 6.1. So in this case I would recommend either to check the version of sidekiq in the #setup! method and decide how to pass / setup the Fetcher or try another approach like to have a special #setup method for sidekiq >= 6.1. I would like to have an opinion from a contributer / maintainer about this. What do you think @ixti ? ✌️

gnomex commented 4 years ago

Hey guys, I'm waiting for this merge, there is anything I can help you to merge as soon as possible?

currently using as

git 'https://github.com/9feet/sidekiq-throttled', branch: 'fixup/fetcher-init-sidekiq-6.1.0' do
  gem "sidekiq-throttled"
end
hmaack commented 4 years ago

Hi @ixti , thx for your suggestions. I switched all conditional checks to Gem::Version.new(Sidekiq::VERSION) < Gem::Version.new("6.1.0") because I got confused after reviewing the different styles in the code. I hope thats OK for you. Check bd2b2e0 and 0307564 they were also wrong at some points. So I tried to use a uniform approach.

ixti commented 4 years ago

Squash-merged in 700507c

ixti commented 4 years ago

Released as v0.13.0