Scheduling catalog items #17594

bdunne · 2018-06-15T16:21:08Z

Based on: #17588

AllenBW · 2018-06-19T17:48:40Z

Hello void, any chance we can get this in? Its holding up

this: ManageIQ/manageiq-api#400

which is holding up

this: ManageIQ/manageiq-v2v#401

AllenBW · 2018-06-19T18:04:18Z

This will also need to be backported Gaprindashvili

carbonin

@gtanzillo can you take a look at the concern I have around Rbac here?

I don't have enough info about the bigger picture to know if that's okay or not.

carbonin · 2018-06-19T19:38:41Z

spec/models/miq_schedule_spec.rb

+          resource = FactoryGirl.create(:host)
+          schedule = FactoryGirl.create(:miq_schedule, :towhat => resource.class.name, :resource_id => resource, :sched_action => {:method => "test_method", :args => ["abc", 123, :a => 1]})
+
+          expect_any_instance_of(Host).to receive("test_method").once.with("abc", 123, :a => 1)


Couldn't we avoid this with something like this:

host = double expect(Host).to receive(:find).with(resource.id).and_return(host) expect(host).to receive("test_method").once.with("abc", 123, :a => 1)

carbonin · 2018-06-19T19:50:45Z

app/models/miq_schedule.rb

@@ -93,6 +93,9 @@ def self.queue_scheduled_work(id, _rufus_job_id, at, _params)

      _log.info("Queueing start of schedule id: [#{id}] [#{sched.name}] [#{sched.towhat}] [#{method}]...complete")
      msg
+    elsif sched.resource.respond_to?(method)
+      sched.resource.send(method, *sched.sched_action[:args])


Doesn't this bypass the RBAC check that is done normally in get_targets? Is that okay?

It's ok because in the other mode (invoke_actions), the targets of the schedule are dynamic and late binding and need to go through RBAC. In this mode, the single target was defined with the schedule and would have gone through RBAC at the time the schedule was created.

carbonin · 2018-06-19T19:59:19Z

app/models/service_template.rb

-    raise result[:errors].join(", ") if result[:errors].any?
-    result[:request]
+    result = order(user, options, request_options)
+    result[:request] || raise(result[:errors].join(", "))


Will there never be errors if the :request key has a value? It seems like that case was reversed in priority here.

Previously, this method would raise if there were any errors even if there was a value for the :request key, now if there is a request value the errors will be ignored. Not sure if that change was intentional or not.

Good point @carbonin, this also concerns me.

After a look it does look like the result will only ever have a request if there are no errors, but because this change didn't have anything to do with the intention of the PR I'm going to move it back to the way it was.

carbonin · 2018-06-19T20:08:54Z

@AllenBW if this needs to get backported it will need a BZ associated with it. Do you have one?

AllenBW · 2018-06-19T20:14:58Z

I do NOOOOOOOOOT 😭 @bthurber yah think we can get a bz asking for the scheduling of migrations in the Gaprindashvili branch?

OR

maybe this isn't work we want to see in the g release?

bthurber · 2018-06-20T01:59:53Z

@AllenBW this isn't a feature for tech preview but is for GA. The RFE BZ is: https://bugzilla.redhat.com/show_bug.cgi?id=1564255

carbonin · 2018-06-20T13:17:43Z

this isn't a feature for tech preview but is for GA

I'm not sure what this means. In the context of ManageIQ this sounds like this shouldn't be backported as the next GA release is Hammer.
If you're referencing some other release timeline we still need the BZ to track backports to the ManageIQ Gaprindashvili branch and that BZ has the flags which indicate it should be backported.

So, put more simply, is this going back to Gaprindashvili or is it just for GA (Hammer only)?

carbonin · 2018-06-20T13:22:19Z

@gtanzillo since @bdunne is out for the next 1.5 weeks do you want me to make the changes in my review and push to this PR?

Then I'll assign to you for final review/merge.

gtanzillo · 2018-06-20T13:24:49Z

@carbonin

@gtanzillo since @bdunne is out for the next 1.5 weeks do you want me to make the changes in my review and push to this PR?

Then I'll assign to you for final review/merge.

That would be great, was going to ask you if you could do that. Thanks ❤️

The inner variable was shadowing the outer one, `hash` was poorly named and the setup can be moved into one `before` block.

carbonin · 2018-06-20T16:08:01Z

Okay, @gtanzillo I think this is good for review now.

miq-bot · 2018-06-20T16:12:46Z

Checked commits bdunne/manageiq@7f692d7~...7a64f16 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

app/models/service_template.rb

❗ - Line 417, Col 34 - Rails/Present - Use if errors.present? instead of unless errors.blank?.

gtanzillo

Looks good! 👍 Thanks @bdunne, @carbonin

simaishi · 2018-07-25T14:22:02Z

@bdunne please add BZ link.

bdunne · 2018-07-25T14:23:37Z

https://bugzilla.redhat.com/show_bug.cgi?id=1564255

simaishi · 2018-07-25T14:24:01Z

Also, this is based on #17588, and PR 17588 is based on #17581. So they all need to go to Gaprindashvili together?

bdunne · 2018-07-25T14:25:59Z

Yes, unfortunately all of these enhancements need to go back for v2v.

simaishi · 2018-07-25T21:35:30Z

@bdunne The spec file conflicts on backport because of #17641. Can you please create a separate PR for this?

simaishi · 2018-07-26T21:13:53Z

Backported to Gaprindashvili via #17765

bdunne changed the title ~~Scheduling catalog items~~ [WIP] Scheduling catalog items Jun 15, 2018

miq-bot added the wip label Jun 15, 2018

bdunne force-pushed the scheduling_catalog_items branch from 6821a01 to e55964e Compare June 15, 2018 16:53

bdunne changed the title ~~[WIP] Scheduling catalog items~~ Scheduling catalog items Jun 15, 2018

miq-bot removed the wip label Jun 15, 2018

bdunne force-pushed the scheduling_catalog_items branch 2 times, most recently from 6d625ea to 1faff36 Compare June 15, 2018 20:31

bdunne mentioned this pull request Jun 15, 2018

Allow ordering ServiceTemplates with a schedule_time ManageIQ/manageiq-api#400

Merged

carbonin reviewed Jun 19, 2018

View reviewed changes

carbonin requested a review from gtanzillo June 19, 2018 20:07

carbonin assigned gtanzillo Jun 19, 2018

carbonin added the enhancement label Jun 19, 2018

bdunne and others added 4 commits June 20, 2018 12:05

Refactor ordering a service template, allow scheduling and queueing

7f692d7

Remove expect_any_instance_of calls in miq_schedule_spec

f246d59

Remove unused spec variables

08bf0f3

Remove duplicate resource_action_workflow and simplify stubbing

7a64f16

The inner variable was shadowing the outer one, `hash` was poorly named and the setup can be moved into one `before` block.

carbonin force-pushed the scheduling_catalog_items branch from 1faff36 to 7a64f16 Compare June 20, 2018 16:06

AllenBW mentioned this pull request Jun 20, 2018

[#369] Inactive migration plans can be scheduled to run ManageIQ/manageiq-v2v#401

Merged

gtanzillo approved these changes Jun 21, 2018

View reviewed changes

gtanzillo added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 21, 2018

gtanzillo merged commit 245ce8b into ManageIQ:master Jun 21, 2018

bdunne deleted the scheduling_catalog_items branch July 3, 2018 16:02

bdunne added the gaprindashvili/yes label Jul 5, 2018

This was referenced Jul 25, 2018

Ensure MiqSchedule#name is unique for ServiceTemplate orders #17696

Merged

Fix class name for queueing #17717

Merged

gmcculloug added the v2v label Jul 25, 2018

simaishi added the blocker label Jul 25, 2018

bdunne mentioned this pull request Jul 25, 2018

[DEPENDS ON ManageIQ/manageiq#17672] Add ServiceTemplate#schedules subcollection ManageIQ/manageiq-api#412

Merged

simaishi added the gaprindashvili/conflict label Jul 26, 2018

bdunne mentioned this pull request Jul 26, 2018

[GAPRINDASHVILI] Scheduling catalog items #17765

Merged

simaishi removed the gaprindashvili/conflict label Jul 26, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jul 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduling catalog items #17594

Scheduling catalog items #17594

bdunne commented Jun 15, 2018

AllenBW commented Jun 19, 2018

AllenBW commented Jun 19, 2018

carbonin left a comment

carbonin Jun 19, 2018

carbonin Jun 19, 2018

gtanzillo Jun 20, 2018

carbonin Jun 19, 2018

gtanzillo Jun 20, 2018

carbonin Jun 20, 2018

carbonin commented Jun 19, 2018

AllenBW commented Jun 19, 2018 •

edited

Loading

bthurber commented Jun 20, 2018

carbonin commented Jun 20, 2018

carbonin commented Jun 20, 2018

gtanzillo commented Jun 20, 2018

carbonin commented Jun 20, 2018

miq-bot commented Jun 20, 2018

gtanzillo left a comment

simaishi commented Jul 25, 2018

bdunne commented Jul 25, 2018

simaishi commented Jul 25, 2018

bdunne commented Jul 25, 2018

simaishi commented Jul 25, 2018

simaishi commented Jul 26, 2018

Scheduling catalog items #17594

Scheduling catalog items #17594

Conversation

bdunne commented Jun 15, 2018

AllenBW commented Jun 19, 2018

AllenBW commented Jun 19, 2018

carbonin left a comment

Choose a reason for hiding this comment

carbonin Jun 19, 2018

Choose a reason for hiding this comment

carbonin Jun 19, 2018

Choose a reason for hiding this comment

gtanzillo Jun 20, 2018

Choose a reason for hiding this comment

carbonin Jun 19, 2018

Choose a reason for hiding this comment

gtanzillo Jun 20, 2018

Choose a reason for hiding this comment

carbonin Jun 20, 2018

Choose a reason for hiding this comment

carbonin commented Jun 19, 2018

AllenBW commented Jun 19, 2018 • edited Loading

bthurber commented Jun 20, 2018

carbonin commented Jun 20, 2018

carbonin commented Jun 20, 2018

gtanzillo commented Jun 20, 2018

carbonin commented Jun 20, 2018

miq-bot commented Jun 20, 2018

gtanzillo left a comment

Choose a reason for hiding this comment

simaishi commented Jul 25, 2018

bdunne commented Jul 25, 2018

simaishi commented Jul 25, 2018

bdunne commented Jul 25, 2018

simaishi commented Jul 25, 2018

simaishi commented Jul 26, 2018

AllenBW commented Jun 19, 2018 •

edited

Loading