Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make auto schedule loading compatible with Array format #345

Merged
merged 4 commits into from
Jul 12, 2022

Conversation

machisuke
Copy link
Contributor

@machisuke machisuke commented Jul 11, 2022

fixes #346

background

The auto-loading introduced in #337 supports Hash format schedule, but not Array format.

For example, with the bellow config/schedule.yml,

-
  name: 'my_first_job'
  class: 'MyFirstJob'
  cron: '1 * * * *'
  description: |
    my_first_job
-
  name: 'my_second_job'
  class: 'MySecondJob'
  cron: '2 * * * *'
  description: |
    my_second_job

the auto-loading causes the below error.

2022-07-11T08:58:27.831Z pid=22 tid=7ge WARN: NoMethodError: undefined method `[]=' for nil:NilClass
2022-07-11T08:58:27.831Z pid=22 tid=7ge WARN: /usr/local/bundle/gems/sidekiq-cron-1.6.0/lib/sidekiq/cron/job.rb:152:in `block in load_from_hash'
/usr/local/bundle/gems/sidekiq-cron-1.6.0/lib/sidekiq/cron/job.rb:151:in `each'
/usr/local/bundle/gems/sidekiq-cron-1.6.0/lib/sidekiq/cron/job.rb:151:in `inject'
/usr/local/bundle/gems/sidekiq-cron-1.6.0/lib/sidekiq/cron/job.rb:151:in `load_from_hash'
/usr/local/bundle/gems/sidekiq-cron-1.6.0/lib/sidekiq/cron/schedule_loader.rb:11:in `block (2 levels) in <main>'
/usr/local/bundle/gems/sidekiq-6.5.1/lib/sidekiq/component.rb:56:in `block in fire_event'
/usr/local/bundle/gems/sidekiq-6.5.1/lib/sidekiq/component.rb:55:in `each'
/usr/local/bundle/gems/sidekiq-6.5.1/lib/sidekiq/component.rb:55:in `fire_event'
/usr/local/bundle/gems/sidekiq-6.5.1/lib/sidekiq/cli.rb:105:in `run'
/usr/local/bundle/gems/sidekiq-6.5.1/bin/sidekiq:31:in `<top (required)>'
bin/sidekiq:29:in `load'
bin/sidekiq:29:in `<main>'

What

This PR solves the above problem so that both Hash- and Array-style definitions are auto-loaded without error.

@markets
Copy link
Member

markets commented Jul 11, 2022

That makes sense to me! ✅

@sandip-mane @danilogco are you fine with that too?

@machisuke machisuke force-pushed the fix_autoload branch 2 times, most recently from 10655a2 to 7c51451 Compare July 11, 2022 11:53
@sandip-mane
Copy link
Contributor

@markets the implementation looks good.

But I feel that instead of supporting the arrays through yml, we should raise an error for invalid hash format.

My opinion is we support only one format, either hash or arrays. Reason being: I don't like the idea of adding both these syntaxes to README it will create confusion.

What do you think?

@markets
Copy link
Member

markets commented Jul 11, 2022

@sandip-mane thanks for your input! I get your point, but I think it feels even more natural to support both formats in the autoloader. Since we have 2 ways of loading jobs from the YAML, supporting both seems least surprising.

@sandip-mane
Copy link
Contributor

Since we have 2 ways of loading jobs from the YAML, supporting both seems least surprising.

@markets sounds reasonable 👍

@markets markets merged commit a2284f2 into sidekiq-cron:master Jul 12, 2022
@machisuke machisuke deleted the fix_autoload branch July 13, 2022 07:04
@markets markets mentioned this pull request Jul 22, 2022
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.

Auto-loading config/schedule.yml feature causes an error with Array style schedule.yml definition.
3 participants