Skip to content

Support crons with timezone for sidekiq-scheduler#2209

Merged
sl0thentr0py merged 2 commits intomasterfrom
neel/sidekiq-scheduler-timezone
Dec 27, 2023
Merged

Support crons with timezone for sidekiq-scheduler#2209
sl0thentr0py merged 2 commits intomasterfrom
neel/sidekiq-scheduler-timezone

Conversation

@sl0thentr0py
Copy link
Copy Markdown
Member

closes #2187

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 27, 2023

Codecov Report

Merging #2209 (3c134d0) into master (d02cf27) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2209   +/-   ##
=======================================
  Coverage   97.39%   97.39%           
=======================================
  Files         101      101           
  Lines        3799     3805    +6     
=======================================
+ Hits         3700     3706    +6     
  Misses         99       99           
Components Coverage Δ
sentry-ruby 98.07% <ø> (ø)
sentry-rails 94.98% <ø> (ø)
sentry-sidekiq 94.70% <100.00%> (+0.17%) ⬆️
sentry-resque 93.65% <ø> (ø)
sentry-delayed_job 94.44% <ø> (ø)
sentry-opentelemetry 100.00% <ø> (ø)
Files Coverage Δ
...-sidekiq/lib/sentry/sidekiq-scheduler/scheduler.rb 93.10% <100.00%> (+1.79%) ⬆️

Copy link
Copy Markdown
Contributor

@bitsandfoxes bitsandfoxes left a comment

Choose a reason for hiding this comment

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

:shipit:

Co-authored-by: Stefan Jandl <reg@bitfox.at>
@sl0thentr0py sl0thentr0py merged commit 2f9ffaf into master Dec 27, 2023
@sl0thentr0py sl0thentr0py deleted the neel/sidekiq-scheduler-timezone branch December 27, 2023 18:52
@github-actions
Copy link
Copy Markdown

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Support crons with timezone for sidekiq-scheduler ([#2209](https://github.com/getsentry/sentry-ruby/pull/2209))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 9b24de9

end
end

class HappyWorkerForCron < HappyWorker; end
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.

Why do we need another HappyWorkerForCron?

Copy link
Copy Markdown
Member Author

@sl0thentr0py sl0thentr0py Jan 2, 2024

Choose a reason for hiding this comment

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

to keep each class isolated after patching, since we don't have a 'reverse prepend'

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 I mean we already had this worker before this PR, so now we have 2:

class HappyWorkerForCron < HappyWorker; end
class HappyWorkerForCron < HappyWorker; end

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ah hmm that was probably copypasta error, thx

monitor_config = case interval_type
when "cron"
Sentry::Cron::MonitorConfig.from_crontab(schedule)
parsed_cron = ::Fugit.parse_cron(schedule)
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.

Can we mention the origin of Fugit in a comment? It's a second-level dependency of sidekiq-scheduler, through the rufus-scheduler gem.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.

Support extracting timezone out of crontab in sidekiq-scheduler

3 participants