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

Replace parse-cron with fugit #710

Merged
merged 1 commit into from
May 10, 2023
Merged

Conversation

adamruzicka
Copy link
Contributor

@adamruzicka adamruzicka commented Mar 1, 2023

parse-cron seems to be unmaintained since 2016, fugit seems to be still active. In addition to that, it should also allow expressing intervals such as "first Monday each month" which wasn't possible with parse-cron.

TODO:

  • packaging

foreman-tasks.gemspec Outdated Show resolved Hide resolved
@adamruzicka adamruzicka marked this pull request as ready for review March 21, 2023 14:00
parse-cron seems to be unmaintained since 2016, fugit seems to be still
active. In addition to that, it should also allow expressing intervals
such as "first Monday each month" which wasn't possible with parse-cron.
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Dependency should be available: theforeman/foreman-packaging#9208. I suspect packit fails because fugit is not listed as a BuildRequires, but we can't deal with that on EL8. For EL9 we may have theforeman/foreman-packaging#8414, but for now we'll have to accept the limitation.

@evgeni perhaps we can patch the spec file based on the gem by autoregenerating the deps, similar to what we do in bump_rpm.sh.

@evgeni
Copy link
Member

evgeni commented May 9, 2023

@ekohl we certainly can (and packit has a hook for that), just nobody sat down and did innocent whistle

@adamruzicka adamruzicka merged commit 7d4ed59 into theforeman:master May 10, 2023
@adamruzicka adamruzicka deleted the fugit branch May 10, 2023 11:20
@adamruzicka
Copy link
Contributor Author

Thank you @ekohl !

@sjha4
Copy link

sjha4 commented May 11, 2023

I am seeing some change in behavior with fugit.

[1] pry(main)> require 'fugit'
=> false
[2] pry(main)> @parser ||= Fugit.parse_cron('* * * * abc')
=> nil
[3] pry(main)> @parser ||= Fugit.parse_cron(nil)
=> nil
[4] pry(main)> @parser ||= Fugit.parse_cron('* * * * *')
=> #<Fugit::Cron:0x000056255ad0a590 @cron_s=nil, @day_and=nil, @hours=nil, @minutes=nil, @monthdays=nil, @months=nil, @original="* * * * *", @seconds=[0], @timezone=nil, @weekdays=nil, @zone=nil>

What that does is have nil failures in next_occurrence_time method. Here is the workflow that raises that..

[5] pry(main)> recurring_logic = ::ForemanTasks::RecurringLogic.new_from_cronline('* * * * abc')
=> #<ForemanTasks::RecurringLogic:0x000056255ac31fb0 id: nil, cron_line: "* * * * abc", end_time: nil, max_iteration: nil, iteration: 0, task_group_id: nil, state: nil, triggering_id: nil, purpose: nil>
[6] pry(main)> recurring_logic.valid_cronline?
NoMethodError: undefined method `previous_time' for nil:NilClass
from /home/vagrant/foreman/.vendor/ruby/2.7.0/gems/foreman-tasks-8.0.0/app/models/foreman_tasks/recurring_logic.rb:101:in `next_occurrence_time'

@sjha4
Copy link

sjha4 commented May 11, 2023

Please see: #717 and let me know if that makes sense.

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.

5 participants