-
Notifications
You must be signed in to change notification settings - Fork 897
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
Use MiqSchedule for delayed MiqRequest execution #17520
Use MiqSchedule for delayed MiqRequest execution #17520
Conversation
db07016
to
772c659
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM. @gmcculloug Any thoughts?
app/models/miq_request.rb
Outdated
start_time = get_option(:schedule_time).utc rescue nil | ||
|
||
MiqSchedule.create!( | ||
:name => "Request scheduled", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gmcculloug you mentioned using the name here and I considered that, but I didn't see MiqRequest#name
and didn't want to rely on source.name
being available since source is polymorphic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property to use here would be MiqRequest#description
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay thanks, done
if get_option(:schedule_type) == "schedule" | ||
deliver_on = get_option(:schedule_time).utc rescue nil | ||
start_time = get_option(:schedule_time).utc rescue nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self what if start_time is in the past (aka scheduled in the future but approval took longer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend that we refactor this line. The classic UI sets get_option(:schedule_time)
to a ActiveSupport::TimeWithZone
object which supports the utc
call.
I suspect that if this is set from the API or some other means it would set the time as a string in which case the code will silently error and use nil
. It is also a good opportunity to get rid of the inline rescue as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed by: bb29981
Instead of using MiqQueue with deliver_on to run an MiqRequest later, create a run-once MiqSchedule record.
772c659
to
08a2adf
Compare
Checked commit agrare@08a2adf with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 app/models/miq_request.rb
|
Closing in favor of #17542 |
Instead of using MiqQueue with deliver_on to run an MiqRequest later,
create a run-once MiqSchedule record.