Move create_service_provision_request into MiqAeServiceMethods for consistency #34

ghost · 2017-06-02T09:38:00Z

Fixes #33
https://bugzilla.redhat.com/show_bug.cgi?id=1457754

Related to:
ManageIQ/manageiq-content#126

…n_engine/engine/miq_ae_method_service/miq_ae_service_methods.rb

gmcculloug · 2017-06-02T14:14:00Z

@fdupont-redhat Thanks for the PR and for referencing the BZ.

A few quick comments.

There is no need to create a git issue to link the PR to, a PR is essentially a git issue so you are just duplicating your efforts.
If you do have a git issue using Fixes #nn will automatically close the issue when the PR is merged. (I updated the description.)

In this case I would say the description you provided in the git issue should really be the description of this PR. Again, I updated the description to reflect that.

Will need to modify the existing caller of this method: https://github.com/ManageIQ/manageiq-content/blob/master/content/automate/ManageIQ/System/Request.class/__methods__/order_ansible_playbook.rb#L19

I still need to look into failing tests.

ghost · 2017-06-02T14:42:09Z

@gmcculloug Thanks for these advices.
For the file to modify in manageiq-content, how do you handle this cross linkage?

gmcculloug · 2017-06-02T14:51:19Z

Create a PR in the content repo and in the title and description you should include the dependency.

Here is an example of the title: ManageIQ/manageiq#13049
And description: ManageIQ/manageiq#15251

Once the new PR is created I also like to update the description in this PR for easy reference.

mkanoor · 2017-06-02T19:00:45Z

spec/engine/miq_ae_method_service/miq_ae_service_methods_spec.rb

+        expect(svc_template).to receive(:instance_variable_get).with('@object').and_return(template)
+        expect(template).to receive(:provision_request).with(user, svc_options).and_return(miq_request)
+
+        result = miq_ae_service.execute(:create_service_provision_reques, svc_template, svc_options)


@fdupont-redhat
Is this missing a 't' create_service_provision_request

Good catch @mkanoor. Any thoughts on why the test is failing?

@mkanoor, aka. Hawk Eye ;)
I corrected it. It seems the problem comes from the fact that MiqAeMethodService class is not know in this context. I also tried to change the test to:

it "#create_service_provision_request" do options = {:fred => :flinstone} svc_options = {:dialog_style => "medium"} template = FactoryGirl.create(:service_template_ansible_playbook) miq_request = FactoryGirl.create(:service_template_provision_request) svc_template = MiqAeMethodService::MiqAeServiceServiceTemplate.find(template.id) workspace = double("MiqAeEngine::MiqAeWorkspaceRuntime", :root => options, :persist_state_hash => {}, :ae_user => @user) #miq_ae_service = MiqAeMethodService.new(workspace) allow(workspace).to receive(:disable_rbac) expect(svc_template).to receive(:instance_variable_get).with('@object').and_return(template) expect(template).to receive(:service_provision_request).with(@user, svc_options).and_return(miq_request) result = invoke_ae.execute(:create_service_provision_request, svc_template.inspect, svc_options.inspect) expect(result).to be_kind_of(MiqAeMethodService::MiqAeServiceMiqRequest) end

But invoke_ae doesn't really give $evm object and tells me

undefined method `execute' for #<MiqAeEngine::MiqAeWorkspaceRuntime:0x0000000e3db518>

That is probably why there is no test for create_automation_request and create_provision_request. Should we move these methods to lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service.rb ? That would also be consistent with $evm.create_notification. And it would be easier to write tests. The drawback is that it would probably require more changes to manageiq-content repository.

What do you think @gmcculloug, @mkanoor ?

This should get things moving for the test:

it "create service request" do allow(workspace).to receive(:disable_rbac) expect_any_instance_of(ServiceTemplate).to receive(:provision_request).with(user, svc_options).and_return(miq_request) result = miq_ae_service.execute(:create_service_provision_request, svc_template, svc_options) expect(result).to be_kind_of(MiqAeMethodService::MiqAeServiceMiqRequest) end

gmcculloug

@fdupont-redhat A few updates to get the code working. I did not get time to spend on the test yet. Hopefully can get there in the next day or two.

gmcculloug · 2017-06-06T12:35:23Z

spec/engine/miq_ae_method_service/miq_ae_service_methods_spec.rb

+               :persist_state_hash => {},
+               :ae_user            => user)
+      end
+      let(:miq_ae_service) { MiqAeService.new(workspace) }


You need to add the namespace to the class:
let(:miq_ae_service) { MiqAeMethodService::MiqAeService.new(workspace) }

gmcculloug · 2017-06-06T13:35:25Z

lib/miq_automation_engine/engine/miq_ae_method_service/miq_ae_service_methods.rb

+    def self.create_service_provision_request(svc_template, options = nil)
+      result = ar_object(svc_template).provision_request(@workspace.ae_user, options)
+      MiqAeServiceModelBase.wrap_results(result)
+    end


This method has to change a bit more now that it is running under a different class. Since we do not have access to @workspace we can get the user from the User.current_user set but the calling method execute here.

def self.create_service_provision_request(svc_template, options = nil) result = svc_template.object_send(:provision_request, User.current_user, options) MiqAeServiceModelBase.wrap_results(result) end

…der_ansible_playbook.rb and associated spec to match ManageIQ/manageiq-automation_engine#34

ghost · 2017-06-06T21:32:21Z

@gmcculloug you rock!

gmcculloug · 2017-06-09T18:42:40Z

@fdupont-redhat Let's make one last change to avoid the rubocop warning about using expect_any_instance_of.

I used that call initially because the service model loads it's own copy of the service_template and the expect(template).to receive(:provision_request) would be called on the wrong copy.

To avoid using expect_any_instance_of we just need to return the same template instance when the service model for ServiceTemplate performs a find to look up the instance.

Here is my diff that changes the test to use the same instance of service_template:

-        expect_any_instance_of(ServiceTemplate).to receive(:provision_request).with(user, svc_options).and_return(miq_request)
+        allow(ServiceTemplate).to receive(:find).with(template.id).and_return(template)
+        expect(template).to receive(:provision_request).with(user, svc_options).and_return(miq_request)

miq-bot · 2017-06-12T07:54:01Z

Checked commits fabiendupont/manageiq-automation_engine@f81a347~...2fd5605 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

gmcculloug

@mkanoor Can you give this a final review. Thanks.

mkanoor · 2017-06-12T13:41:25Z

@gmcculloug @fdupont-redhat 👍

simaishi · 2017-06-13T17:04:54Z

Fine backport (to manageiq repo) details:

$ git log -1
commit ebd69905757ba17d786876ebc18c93fd8df6cda4
Author: Greg McCullough <[email protected]>
Date:   Mon Jun 12 09:44:46 2017 -0400

    Merge pull request #34 from fdupont-redhat/bz1447754
    
    Move create_service_provision_request into MiqAeServiceMethods for consistency
    (cherry picked from commit f650a66f1241de078ffdca508b0e77afb8eb5460)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1461144

Move the create_service_provision_request method to lib/miq_automatio…

f81a347

…n_engine/engine/miq_ae_method_service/miq_ae_service_methods.rb

miq-bot added the wip label Jun 2, 2017

gmcculloug requested a review from mkanoor June 2, 2017 12:25

gmcculloug self-assigned this Jun 2, 2017

mkanoor reviewed Jun 2, 2017

View reviewed changes

gmcculloug reviewed Jun 6, 2017

View reviewed changes

ghost pushed a commit to fabiendupont/manageiq-content that referenced this pull request Jun 6, 2017

Updated content/automate/ManageIQ/System/Request.class/__methods__/or…

d738eea

…der_ansible_playbook.rb and associated spec to match ManageIQ/manageiq-automation_engine#34

ghost mentioned this pull request Jun 6, 2017

Update method order_ansible_playbook.rb to match the move of create_service_provision_request ManageIQ/manageiq-content#126

Merged

Updated following @gmcculloug recommendations

18e4d1d

Updated spec to get rid of rubocop complains

2fd5605

gmcculloug changed the title ~~[WIP] Move create_service_provision_request into MiqAeServiceMethods for consistency~~ Move create_service_provision_request into MiqAeServiceMethods for consistency Jun 12, 2017

gmcculloug added fine/yes technical debt and removed wip labels Jun 12, 2017

gmcculloug approved these changes Jun 12, 2017

View reviewed changes

mkanoor approved these changes Jun 12, 2017

View reviewed changes

gmcculloug merged commit f650a66 into ManageIQ:master Jun 12, 2017

gmcculloug added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 12, 2017

simaishi added fine/backported and removed fine/yes labels Jun 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move create_service_provision_request into MiqAeServiceMethods for consistency #34

Move create_service_provision_request into MiqAeServiceMethods for consistency #34

ghost commented Jun 2, 2017 •

edited by gmcculloug

Loading

gmcculloug commented Jun 2, 2017

ghost commented Jun 2, 2017

gmcculloug commented Jun 2, 2017

mkanoor Jun 2, 2017

gmcculloug Jun 2, 2017

ghost Jun 3, 2017

ghost Jun 4, 2017

gmcculloug Jun 6, 2017

gmcculloug left a comment

gmcculloug Jun 6, 2017

gmcculloug Jun 6, 2017

ghost commented Jun 6, 2017

gmcculloug commented Jun 9, 2017

miq-bot commented Jun 12, 2017

gmcculloug left a comment

mkanoor commented Jun 12, 2017

simaishi commented Jun 13, 2017

Move create_service_provision_request into MiqAeServiceMethods for consistency #34

Move create_service_provision_request into MiqAeServiceMethods for consistency #34

Conversation

ghost commented Jun 2, 2017 • edited by gmcculloug Loading

gmcculloug commented Jun 2, 2017

ghost commented Jun 2, 2017

gmcculloug commented Jun 2, 2017

mkanoor Jun 2, 2017

Choose a reason for hiding this comment

gmcculloug Jun 2, 2017

Choose a reason for hiding this comment

ghost Jun 3, 2017

Choose a reason for hiding this comment

ghost Jun 4, 2017

Choose a reason for hiding this comment

gmcculloug Jun 6, 2017

Choose a reason for hiding this comment

gmcculloug left a comment

Choose a reason for hiding this comment

gmcculloug Jun 6, 2017

Choose a reason for hiding this comment

gmcculloug Jun 6, 2017

Choose a reason for hiding this comment

ghost commented Jun 6, 2017

gmcculloug commented Jun 9, 2017

miq-bot commented Jun 12, 2017

gmcculloug left a comment

Choose a reason for hiding this comment

mkanoor commented Jun 12, 2017

simaishi commented Jun 13, 2017

ghost commented Jun 2, 2017 •

edited by gmcculloug

Loading