Add zone to q_options in call_automate. #18

tinaafitz · 2017-05-08T19:07:31Z

Separate Manageiq change to pass zone_name required.

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

mkanoor · 2017-05-08T19:28:37Z

spec/miq_ae_event_spec.rb


-        it "has tenant" do
+        it "raise_evm_event, deliver_queue, with zone" do


@tinaafitz can we use a shared example for these 2 tests and just change the zones

mkanoor · 2017-05-08T19:32:54Z

spec/miq_ae_event_spec.rb

@@ -80,7 +101,7 @@
        it "has tenant" do
          args = {:user_id => admin.id, :miq_group_id => vm_group.id, :tenant_id => vm_group.tenant.id}
          expect(MiqAeEngine).to receive(:deliver_queue).with(hash_including(args), anything)
-
+          expect(MiqServer).to receive(:my_zone).and_return('default')


@tinaafitz
Can we have something like this
before { allow(MiqServer).to receive_messages(:my_zone => "default") }
then we dont have to change every line

@mkanoor As discussed, will refactor this test in a separate PR.

gmcculloug · 2017-05-08T21:46:34Z

lib/miq_automation_engine/engine/miq_ae_event.rb

@@ -121,6 +121,7 @@ def self.call_automate(obj, attrs, instance_name, options = {})
    raise "A user is needed to raise #{instance_name} to automate. [#{obj.class.name}] id:[#{obj.id}]" unless user

    q_options = {
+      :zone         => Zone.determine_queue_zone(options),


@tinaafitz This is already called during MiqQueue.put? Is this really needed?

gmcculloug · 2017-05-09T16:15:29Z

lib/miq_automation_engine/engine/miq_ae_event.rb

@@ -125,6 +125,7 @@ def self.call_automate(obj, attrs, instance_name, options = {})
      :priority     => MiqQueue::HIGH_PRIORITY,
      :task_id      => nil          # Clear task_id to allow running synchronously under current worker process
    }
+    q_options[:zone] = options[:zone] if options.keys.include?(:zone) && !options[:zone].nil?


q_options[:zone] = options[:zone] if options[:zone].present?

gmcculloug · 2017-05-10T12:59:07Z

spec/miq_ae_event_spec.rb

      context "with group owned VM" do
        let(:vm_group) { FactoryGirl.create(:miq_group, :tenant => FactoryGirl.create(:tenant)) }

        it "has tenant" do
          args = {:user_id => admin.id, :miq_group_id => vm_group.id, :tenant_id => vm_group.tenant.id}
          expect(MiqAeEngine).to receive(:deliver_queue).with(hash_including(args), anything)
-


@tinaafitz No sure if you intentionally remove this line but it seems random with the other changes in this PR and now makes this test inconsistent with (most) of the remaining tests that separate the expect().to receive and the following method call. I tend to prefer this separation between testing logic and the code being run for the test.

@gmcculloug Added blank line back, thanks for catching it.

Separate Manageiq change to pass zone_name required. https://bugzilla.redhat.com/show_bug.cgi?id=1447625

miq-bot · 2017-05-10T13:54:22Z

Checked commit tinaafitz@c3cf50f with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

simaishi · 2017-05-10T15:19:31Z

Fine backport (to manageiq repo) details:

$ git log -1
commit 86c23029db18065a2ea5b27d00aac60fc170c518
Author: Greg McCullough <[email protected]>
Date:   Wed May 10 10:23:00 2017 -0400

    Merge pull request #18 from tinaafitz/add_zone_to_qoptions
    
    Add zone to q_options in call_automate.
    (cherry picked from commit 0495c11579d5751f91c6e6791c65cdd1db09ad88)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1449748

simaishi · 2017-05-10T15:27:09Z

Euwe backport (to manageiq repo) details:

$ git log -1
commit 7051ce276be724232cd2d0b4e46b31e8e24f2ab1
Author: Greg McCullough <[email protected]>
Date:   Wed May 10 10:23:00 2017 -0400

    Merge pull request #18 from tinaafitz/add_zone_to_qoptions
    
    Add zone to q_options in call_automate.
    (cherry picked from commit 0495c11579d5751f91c6e6791c65cdd1db09ad88)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1449753

tinaafitz changed the title ~~Add zone to q_options in call_automate.~~ [WIP] Add zone to q_options in call_automate. May 8, 2017

miq-bot added the wip label May 8, 2017

gmcculloug added blocker bug fine/yes labels May 8, 2017

gmcculloug requested a review from mkanoor May 8, 2017 19:16

gmcculloug self-assigned this May 8, 2017

mkanoor reviewed May 8, 2017

View reviewed changes

tinaafitz force-pushed the add_zone_to_qoptions branch from b4020f6 to 96be424 Compare May 8, 2017 19:32

mkanoor reviewed May 8, 2017

View reviewed changes

tinaafitz force-pushed the add_zone_to_qoptions branch from 96be424 to db48545 Compare May 8, 2017 20:48

mkanoor approved these changes May 8, 2017

View reviewed changes

tinaafitz changed the title ~~[WIP] Add zone to q_options in call_automate.~~ Add zone to q_options in call_automate. May 8, 2017

miq-bot removed the wip label May 8, 2017

gmcculloug reviewed May 8, 2017

View reviewed changes

tinaafitz force-pushed the add_zone_to_qoptions branch from db48545 to 03d0263 Compare May 9, 2017 16:01

gmcculloug reviewed May 9, 2017

View reviewed changes

tinaafitz force-pushed the add_zone_to_qoptions branch from 03d0263 to d189964 Compare May 9, 2017 16:23

gmcculloug approved these changes May 9, 2017

View reviewed changes

tinaafitz force-pushed the add_zone_to_qoptions branch 2 times, most recently from 19ddbd6 to 009400b Compare May 9, 2017 22:30

gmcculloug reviewed May 10, 2017

View reviewed changes

Add zone to q_options in call_automate.

c3cf50f

Separate Manageiq change to pass zone_name required. https://bugzilla.redhat.com/show_bug.cgi?id=1447625

tinaafitz force-pushed the add_zone_to_qoptions branch from 009400b to c3cf50f Compare May 10, 2017 13:50

tinaafitz mentioned this pull request May 10, 2017

Change retire_now to pass zone_name to raise_retirement_event. ManageIQ/manageiq#15026

Merged

gmcculloug merged commit 0495c11 into ManageIQ:master May 10, 2017

gmcculloug added this to the Sprint 61 Ending May 22, 2017 milestone May 10, 2017

gmcculloug added the euwe/yes label May 10, 2017

simaishi added fine/backported and removed fine/yes labels May 10, 2017

simaishi added euwe/backported and removed euwe/yes labels May 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add zone to q_options in call_automate. #18

Add zone to q_options in call_automate. #18

tinaafitz commented May 8, 2017 •

edited by gmcculloug

Loading

mkanoor May 8, 2017

mkanoor May 8, 2017

tinaafitz May 8, 2017

gmcculloug May 8, 2017

gmcculloug May 9, 2017

gmcculloug May 10, 2017

tinaafitz May 10, 2017

miq-bot commented May 10, 2017

simaishi commented May 10, 2017 •

edited

Loading

simaishi commented May 10, 2017


		it "has tenant" do
		it "raise_evm_event, deliver_queue, with zone" do

Add zone to q_options in call_automate. #18

Add zone to q_options in call_automate. #18

Conversation

tinaafitz commented May 8, 2017 • edited by gmcculloug Loading

mkanoor May 8, 2017

Choose a reason for hiding this comment

mkanoor May 8, 2017

Choose a reason for hiding this comment

tinaafitz May 8, 2017

Choose a reason for hiding this comment

gmcculloug May 8, 2017

Choose a reason for hiding this comment

gmcculloug May 9, 2017

Choose a reason for hiding this comment

gmcculloug May 10, 2017

Choose a reason for hiding this comment

tinaafitz May 10, 2017

Choose a reason for hiding this comment

miq-bot commented May 10, 2017

simaishi commented May 10, 2017 • edited Loading

simaishi commented May 10, 2017

tinaafitz commented May 8, 2017 •

edited by gmcculloug

Loading

simaishi commented May 10, 2017 •

edited

Loading