Require rails >= 7 #458
Require rails >= 7 #458
Conversation
|
Why the heck is sidekiq-6.0.4 getting pulled in on ruby3.1, while all the other rubies (including 2.7) get 6.5.12. Edit: gitlab-sidekiq-fetcher seems to be pulling that in |
ofedoren
left a comment
ofedoren
left a comment
There was a problem hiding this comment.
Thanks, @adamruzicka, I'm afraid I don't have much input on this, just a note inline.
| Sidekiq::ReliableFetch.setup_reliable_fetch!(config) | ||
| end | ||
| ::Sidekiq.strict_args!(false) | ||
| ::Sidekiq.strict_args!(false) if ::Sidekiq.respond_to?(:strict_args!) |
There was a problem hiding this comment.
Why do we need that check?
Also, looking at what it does, shouldn't we put it something like Rails.dev? || Rails.test? ? :warn : false? Or we don't want to migrate to the strings only args?
There was a problem hiding this comment.
Why do we need that check?
Due to some combination of transitive dependencies, early version of sidekiq-6 was being pulled in in tests, but this early version didn't implement Siedkiq.strict_args yet. Since we try to support the entire sidekiq 6 line, this felt like the easiest way out
Or we don't want to migrate to the strings only args?
Doing this properly (not trying to send non-json-native things anywhere) would be a major undertaking as that would have to happen both in dynflow's internals as well as in the user code being managed by dynflow. A hacky way would be to extend our own serialization layer to serialize even the things which previously didn't need to be.
shouldn't we put it something like Rails.dev? || Rails.test? ? :warn : false
While we could do that, I'm not aware of anyone who would be running sidekiq in development?
There was a problem hiding this comment.
A hacky way would be to extend our own serialization layer to serialize even the things which previously didn't need to be.
Something like https://github.com/Dynflow/dynflow/compare/master...adamruzicka:sidekiq-json?expand=1 maybe? But I can't say I like it
There was a problem hiding this comment.
That's interesting, and at the first glance it should cover everything. It's just that strict_args without an argument will raise an error, so it's kinda dangerous to just rely on "it should". Maybe warn for the starters and if we won't see any, then just drop it in the future?
| Sidekiq::ReliableFetch.setup_reliable_fetch!(config) | ||
| end | ||
| ::Sidekiq.strict_args!(false) | ||
| ::Sidekiq.strict_args!(false) if ::Sidekiq.respond_to?(:strict_args!) |
2 participants
No description provided.