Transformation Plan, Request, and Task for V2V #16960

bzwei · 2018-02-06T18:52:25Z

Introduce new class for migration plan
Introduce new request and task types for execution

Depends on #16787

bzwei · 2018-02-06T18:52:46Z

@miq-bot add_label v2v

bzwei · 2018-02-06T20:30:31Z

cc @fdupont-redhat @lfu
@miq-bot assign @gmcculloug

bzwei · 2018-02-07T18:18:09Z

cc @jntullo

gmcculloug

@bzwei Can we find a point to un-wip this and iterate over multiple PRs before this PR gets to large.

gmcculloug · 2018-02-13T14:13:06Z

app/models/service_template_provision_task.rb

@@ -138,7 +138,7 @@ def deliver_to_automate(req_type = request_type, _zone = nil)
        args[:attrs].merge!(ra.ae_attributes)
      end

-      args[:attrs].merge!(MiqAeEngine.create_automation_attributes(destination.class.base_model.name => destination))
+      args[:attrs].merge!(MiqAeEngine.create_automation_attributes(destination.class.base_model.name => destination)) unless destination.nil?


Prefer if destination.present? over unless logic.

gmcculloug · 2018-02-13T14:13:10Z

app/models/service_template_transformation_plan.rb

+
+  def add_vms(vm_ids)
+    vm_ids.each { |vm_id| add_vm(vm_id) }
+    save!


Can we load the VM objects and use the existing add_resource logic of ServiceTemplate instead of creating our own add_vm method? You may not require the save! if you make this change.

This is purposely to by-pass the constraint that a VM can belong to only one service/service_template.

Does it make more sense to override the enforce_single_service_parent method for this service type?
https://github.com/ManageIQ/manageiq/blob/master/app/models/mixins/service_mixin.rb#L113

For now it could just skip it and in the future be used to control if a VM can be in multiple migration plans.

bzwei · 2018-02-14T22:24:48Z

@miq-bot remove_label wip

bzwei · 2018-02-14T22:26:07Z

wip label removed. Please review.

gmcculloug · 2018-02-15T18:48:42Z

app/models/service_template_provision_task.rb

@@ -158,6 +158,10 @@ def deliver_to_automate(req_type = request_type, _zone = nil)
    end
  end

+  def resource_action
+    source.resource_actions.detect { |ra| ra.action == 'Provision' } if source.respond_to?(:resource_actions)


@bzwei This method name should be more descriptive. My guess is you might be trying to abstract the "Provision" part away from the Transformation models. If that is not correct this could be provision_resource_action.

Otherwise we could use a more generic term like initialize_resource_action or create_resource_action.

If we abstract this method name it would allow the subclass to define a different ra.action value (currently "Provision") by moving the "Provision" value into a constant that the subclass could override.

My understanding ruby's naming convention - resource_action means get_resource_action. I want to abstract the resource action, no matter where it come from. Can be from source or miq_request, or other place, can be type Provision or other type.

I see that you are already overriding this method for the ServiceTemplateTransformationPlanTask class.

At the base class I feel it is misleading to have resource_action return the "Provision" action. It would not be clear to me that resource_action == "Provision" but there are also "Reconfigure" and "Retirement" actions. This is my main objection to the proposed method name.

The base class is ServiceTemplateProvisionTask, so the action has to be Provision. A task can only have one action, for this task Provision is implied.

Right, the piece I was overlooking was "A task can only have one action". Thanks.

gmcculloug · 2018-02-16T20:13:40Z

app/models/service_template_transformation_plan.rb

+    config_info = options[:config_info]
+
+    mapping = if config_info[:transformation_mapping_id]
+                TransformationMapping.find(config_info[:transformation_mapping_id])


Do you mean to raise the RecordNotFound error here or should this return nil and raise on line 73 on the check if mapping.blank?

Yes. I mean to have the RecordNotFound if the caller provides transformation_mapping_id but invalid. The next exception occurs if no transformation_mapping is given at all.

gmcculloug · 2018-02-16T20:14:01Z

app/models/service_template_transformation_plan.rb

+              end
+
+    if mapping.blank?
+      raise _('Must provide an existing transformation mapping')


Convert to a single line

gmcculloug · 2018-02-16T20:14:09Z

app/models/service_template_transformation_plan.rb

+          end
+
+    if vms.blank?
+      raise _('Must select a list of valid vms')


gmcculloug · 2018-02-16T20:27:17Z

app/models/service_template_transformation_plan_request.rb

+    true
+  end
+
+  def approve_vm(vm_id)


Who is the caller of this? Is it the UI (through API?)

automate calls it.

gmcculloug · 2018-02-16T20:43:21Z

spec/models/service_template_transformation_plan_request_spec.rb

+
+  describe 'customize_request_task_attributes' do
+    it 'overrides #customize_request_task_attributes' do
+      expect(described_class.instance_methods(false)).to include(:customize_request_task_attributes)


We should be testing the result of the method and not just the existence of the override.

gmcculloug · 2018-02-16T20:53:23Z

spec/models/service_template_transformation_plan_spec.rb

+  describe '.create_catalog_item' do
+    it 'creates and returns a transformation plan' do
+      service_template = described_class.create_catalog_item(catalog_item_options)
+      service_template.reload


Why is this reloaded needed if the call is returning the object for the first time?

If it is needed move the reload to the end of the first line.

gmcculloug · 2018-02-16T20:55:04Z

spec/models/service_template_transformation_plan_task_spec.rb

+
+  describe '#after_request_task_create' do
+    it 'overrides #after_request_task_create' do
+      expect(described_class.instance_methods(false)).to include(:after_request_task_create)


Same here. We should be testing the result of the method and not just the existence of the override.

miq-bot · 2018-02-19T15:37:14Z

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

Changes made to prepare for introducing new models for V2V service template

This is the model to store a migration plan

miq-bot · 2018-02-19T17:17:53Z

Checked commits bzwei/manageiq@ebc0505~...ee4b78b with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
14 files checked, 0 offenses detected
Everything looks fine. ⭐

bzwei · 2018-02-27T14:59:57Z

@miq-bot add_label enhancement

himdel · 2018-05-29T10:50:19Z

@miq-bot add_label gaprindashvili/yes

(ManageIQ/manageiq-ui-classic#3963: because Could not create Service Template - uninitialized constant ServiceTemplateTransformationPlan when attempting to create a Migration Plan in v2v.)

(looks like the factory merge conflict comes from #16939)

Transformation Plan, Request, and Task for V2V (cherry picked from commit 02af759)

simaishi · 2018-05-29T18:52:03Z

Gaprindashvili backport details:

$ git log -1
commit b272ec13bf625da36d3df9eb817d34a0593722d2
Author: Greg McCullough <[email protected]>
Date:   Mon Feb 19 13:41:15 2018 -0500

    Merge pull request #16960 from bzwei/v2v_service_template
    
    Transformation Plan, Request, and Task for V2V
    (cherry picked from commit 02af759aa7525ea840177178589619fc892853c7)

miq-bot added v2v wip labels Feb 6, 2018

bzwei mentioned this pull request Feb 6, 2018

New service model for v2v request and task ManageIQ/manageiq-automation_engine#155

Merged

miq-bot assigned gmcculloug Feb 6, 2018

bzwei force-pushed the v2v_service_template branch from 34cb4fd to f6c0a0a Compare February 7, 2018 16:28

bzwei force-pushed the v2v_service_template branch 2 times, most recently from 96f24fd to 90f64b9 Compare February 7, 2018 22:12

jntullo mentioned this pull request Feb 8, 2018

[WIP] Add transformation plans read and create ManageIQ/manageiq-api#315

Closed

bzwei force-pushed the v2v_service_template branch 3 times, most recently from 3fe7629 to d3f93fc Compare February 13, 2018 14:10

gmcculloug reviewed Feb 13, 2018

View reviewed changes

bzwei force-pushed the v2v_service_template branch 4 times, most recently from c3df843 to cba456c Compare February 14, 2018 22:22

bzwei changed the title ~~[WIP] initial support for migration plan and request~~ Transformation Plan, Request, and Task for V2V Feb 14, 2018

miq-bot removed the wip label Feb 14, 2018

gmcculloug reviewed Feb 15, 2018

View reviewed changes

gmcculloug reviewed Feb 16, 2018

View reviewed changes

miq-bot added the unmergeable label Feb 19, 2018

bzwei force-pushed the v2v_service_template branch from cba456c to b497025 Compare February 19, 2018 16:29

Refactor service_template and provision code

ebc0505

Changes made to prepare for introducing new models for V2V service template

bzwei added 2 commits February 19, 2018 11:32

Introducing ServiceTemplateTransformationPlan for V2V

43f446e

This is the model to store a migration plan

Introducing ServiceTemplateTransformationPlanRequest and Task for V2V

41ad27f

bzwei force-pushed the v2v_service_template branch from b497025 to 074edae Compare February 19, 2018 16:37

miq-bot removed the unmergeable label Feb 19, 2018

Address review comments

ee4b78b

bzwei force-pushed the v2v_service_template branch from 074edae to ee4b78b Compare February 19, 2018 17:10

gmcculloug approved these changes Feb 19, 2018

View reviewed changes

gmcculloug merged commit 02af759 into ManageIQ:master Feb 19, 2018

gmcculloug added this to the Sprint 80 Ending Feb 26, 2018 milestone Feb 19, 2018

miq-bot added the enhancement label Feb 27, 2018

miq-bot added the gaprindashvili/yes label May 29, 2018

simaishi pushed a commit that referenced this pull request May 29, 2018

Merge pull request #16960 from bzwei/v2v_service_template

b272ec1

Transformation Plan, Request, and Task for V2V (cherry picked from commit 02af759)

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels May 29, 2018

miq-bot added the gaprindashvili/yes label May 29, 2018

simaishi removed the gaprindashvili/yes label May 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transformation Plan, Request, and Task for V2V #16960

Transformation Plan, Request, and Task for V2V #16960

bzwei commented Feb 6, 2018

bzwei commented Feb 6, 2018

bzwei commented Feb 6, 2018

bzwei commented Feb 7, 2018

gmcculloug left a comment

gmcculloug Feb 13, 2018

gmcculloug Feb 13, 2018

bzwei Feb 13, 2018

gmcculloug Feb 13, 2018

bzwei commented Feb 14, 2018

bzwei commented Feb 14, 2018

gmcculloug Feb 15, 2018

bzwei Feb 15, 2018

gmcculloug Feb 15, 2018

bzwei Feb 15, 2018

gmcculloug Feb 15, 2018

gmcculloug Feb 16, 2018

bzwei Feb 19, 2018

gmcculloug Feb 16, 2018

gmcculloug Feb 16, 2018

gmcculloug Feb 16, 2018

bzwei Feb 19, 2018 •

edited

Loading

gmcculloug Feb 16, 2018

gmcculloug Feb 16, 2018

gmcculloug Feb 16, 2018

miq-bot commented Feb 19, 2018

miq-bot commented Feb 19, 2018

bzwei commented Feb 27, 2018

himdel commented May 29, 2018 •

edited

Loading

simaishi commented May 29, 2018

Transformation Plan, Request, and Task for V2V #16960

Transformation Plan, Request, and Task for V2V #16960

Conversation

bzwei commented Feb 6, 2018

bzwei commented Feb 6, 2018

bzwei commented Feb 6, 2018

bzwei commented Feb 7, 2018

gmcculloug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzwei commented Feb 14, 2018

bzwei commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bzwei Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Feb 19, 2018

miq-bot commented Feb 19, 2018

bzwei commented Feb 27, 2018

himdel commented May 29, 2018 • edited Loading

simaishi commented May 29, 2018

bzwei Feb 19, 2018 •

edited

Loading

himdel commented May 29, 2018 •

edited

Loading