resque / resque-web

a Rails-based web interface to Resque
235 stars 166 forks source link

Resque-web should not set the global Resque.redis #122

Closed maxkwallace closed 7 years ago

maxkwallace commented 7 years ago

Hey-- thanks for the awesome gem! We use it all the time to monitor Resque jobs.

That said, I think resque-web should not set Resque.redis. This broke our app today, because we set Resque.redis in a different initializer, using a different environment variable, and the load order changed-- I had no idea that resque-web had a Resque.redis = ... call in an initializer.

I realize this is a breaking change, but it seems a bit strange and dangerous to have resque-web handle this logic, since that's a contract between the user and the Resque gem itself.

What do you think?

grosser commented 7 years ago

@kirillplatonov good to merge ... we ran into the same issue :(

grosser commented 7 years ago

... build error is unrelated and master is red too ... @kirillplatonov if you need maintainer let me know, using this gem in a few projects at zendesk

grosser commented 7 years ago

FYI this blows up on redis 1.27 ... 1.26 ignores it and works fine ...

kirillplatonov commented 7 years ago

@maxkwallace @grosser thanks for that pull request. I think originally it was added for standalone usage of that dashboard. When you use it without resque inside you app and connects to another queue. But actually this method is not described in Readme at all so I don't think we have many such use cases. Probably we shouldn't remove this settings, but rather make it optional and add check if env variable was set?

grosser commented 7 years ago

I'm ok with that ... so basically wrap in if ENV['... ... @maxkwallace got time to change that ... otherwise I can make a quick PR ...

kirillplatonov commented 7 years ago

@grosser I will appreciate any help with this project and probably if you actively use it in production maybe you have some thoughts about what version 1.0.0 should contains (https://github.com/resque/resque-web/issues/100)?

grosser commented 7 years ago

we just use it for basic "look at them jobs" kind of usecase, so more trying to keep the light on then re-architecting :)

On Mon, Mar 6, 2017 at 9:49 PM, Kirill Platonov notifications@github.com wrote:

@grosser https://github.com/grosser I will appreciate any help with this project and probably if you actively use it in production maybe you have some thoughts about what version 1.0.0 should contains (#100 https://github.com/resque/resque-web/issues/100)?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/resque/resque-web/pull/122#issuecomment-284628612, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAsZ-aZDmk6sf-JTV_sOZglkI3MVuPiks5rjPAFgaJpZM4MJcVB .

maxkwallace commented 7 years ago

@grosser @kirillplatonov sounds good! I'll add an if ENV['... check in this PR, and update the changes to the README to take that into account.

Thanks for the consideration and feedback :)

maxkwallace commented 7 years ago

@kirillplatonov PR code updated.

boie0025 commented 7 years ago

👍