Skip to content

Remove sidekiq/job_retry dependency - #43

Merged
kaisen-san merged 3 commits into
enova:mainfrom
stefanste:require-fix-max-retries
Aug 19, 2026
Merged

kaisen-san merged 3 commits into
enova:mainfrom
stefanste:require-fix-max-retries

Conversation

@stefanste

@stefanste stefanste commented Aug 18, 2026 •

Copy link
Copy Markdown
Contributor

I have made an oversight in the recent fix PR #42 and not required this. Without it when the fallback max retry attempts value gets accessed, Sidekiq::JobRetry::DEFAULT_MAX_RETRY_ATTEMPTS is not defined.

Included a spec which fails when the sidekiq/job_retry module is not required.

Thanks for reviewing and merging the previous PR promptly, apologies for missing this!

@stefanste stefanste changed the title Require fix max retries Require sidekiq/job_retry Aug 18, 2026

@kaisen-san kaisen-san left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. Thanks for putting a fix together! Seems like some tests failed, though. Could you look into it?

@stefanste

stefanste commented Aug 18, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks @kaisen-san I've pushed a fix that should sort it.

It was failing due to Sidekiq v4 defining that constant in a different place to 5+: https://github.com/sidekiq/sidekiq/blob/fe43e1cbcd0498a37a69581ace523bc36630cd3f/lib/sidekiq/middleware/server/retry_jobs.rb#L67

We could do something like wrap the require in a a begin/rescue, and then require it from the different location to support Sidekiq 4. But maybe better not to reach into Sidekiq's internals and just define the constant here to match? Let me know what you think.

@kaisen-san

Copy link
Copy Markdown
Contributor

Yeah, that makes sense to me!

@kaisen-san kaisen-san changed the title Require sidekiq/job_retry Remove sidekiq/job_retry dependency Aug 19, 2026
@kaisen-san
kaisen-san merged commit 8022ca8 into enova:main Aug 19, 2026
27 checks passed
@kaisen-san kaisen-san mentioned this pull request Aug 19, 2026
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.

2 participants