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

[WIP] Change MiqRequest to use MiqSchedules for scheduled requests #17542

Closed
wants to merge 7 commits into from

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Jun 6, 2018

While working on the V2V project, there are a few use cases around scheduled requests that need some enhancements. In order to accommodate those use cases, it would be best to use schedules rather than queue items with a deliver on date.

Desired V2V features:

  • Schedule a new request (already exists)
  • Unschedule a request
  • Reschedule a request

This is currently using a reserve column since it needs to be backported. I will create a followup PR here and in manageiq-schema to get out of the reserved attribute.

Follow-ups:
ManageIQ/manageiq-schema#212
#17545

@bdunne
Copy link
Member Author

bdunne commented Jun 6, 2018

This includes the change in #17520

@bdunne bdunne force-pushed the schedule_changes branch from 8d0547f to a443211 Compare June 7, 2018 14:11
@miq-bot
Copy link
Member

miq-bot commented Jun 7, 2018

Some comments on commits bdunne/manageiq@d881e84~...a443211

spec/models/miq_schedule_spec.rb

  • ⚠️ - 717 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Jun 7, 2018

Checked commits bdunne/manageiq@d881e84~...a443211 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 2 offenses detected

app/models/miq_request.rb

spec/models/miq_schedule_spec.rb

@bdunne bdunne mentioned this pull request Jun 7, 2018
if get_option(:schedule_type) == "schedule"
deliver_on = get_option(:schedule_time).utc rescue nil
start_time = get_option(:schedule_time).utc rescue nil
Copy link
Member

Choose a reason for hiding this comment

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

D'oh I meant to put this here

Copying @gmcculloug 's comment (#17520 (comment))

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.

Copy link
Member

Choose a reason for hiding this comment

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

Fixed by #17547

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

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

This looks good to me, I wonder if we should unify the invoke_actions in MiqSchedule eventually or add an action_create_miq_request_tasks so it is consistent with how the rest of the actions are.

@bdunne
Copy link
Member Author

bdunne commented Jun 7, 2018

@agrare Yeah, I was hoping to remove the action_ methods and move any necessary logic to the class that it belongs in.

@bdunne bdunne requested a review from gtanzillo June 7, 2018 17:14
@bdunne bdunne changed the title Change MiqRequest to use MiqSchedules for scheduled requests [WIP] Change MiqRequest to use MiqSchedules for scheduled requests Jun 14, 2018
@miq-bot miq-bot added the wip label Jun 14, 2018
@miq-bot
Copy link
Member

miq-bot commented Jun 15, 2018

This pull request is not mergeable. Please rebase and repush.

@bdunne
Copy link
Member Author

bdunne commented Jun 15, 2018

Closing since v2v is interacting with ServiceTemplates instead of MiqRequests

@bdunne bdunne closed this Jun 15, 2018
@bdunne bdunne deleted the schedule_changes branch June 15, 2018 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants