[V2V] Lan validation in Transformation Mapping #19220

thearifismail · 2019-08-28T19:01:56Z

This PR validates that the source and target networks (lans) are associated with the parent EmsCluster.

Target Lans
RHV: Class Lan, Filters: Must belong to target cluster
OSP: Class CloudNetwork, Filters: Must belong to target CloudTenant's ExtManagementSystem

Source Lans
VMW: Class Lan, Filters: Must belong to one of the source clusters

https://trello.com/c/1x31IdD0/241-spike-mapping-and-plan-api-validation-of-input
https://bugzilla.redhat.com/show_bug.cgi?id=1666840

…ecs for it

thearifismail · 2019-08-28T19:11:59Z

@miq-bot add_reviewers @djberg96, @agrare
@miq-bot add_labels transformation, ivanchuk/yes, hammer/yes

miq-bot · 2019-08-28T19:13:16Z

@thearifismail 'djberg96 agrare' is an invalid reviewer, ignoring...

miq-bot · 2019-08-28T19:15:06Z

@thearifismail 'djberg96, agrare' is an invalid reviewer, ignoring...

thearifismail · 2019-08-28T19:16:34Z

Related PRs:
#19204
ManageIQ/manageiq-api#664
#19168
#18986

djberg96 · 2019-09-03T13:39:14Z

spec/models/transformation_mapping_item_spec.rb

+            vmware_cluster.hosts << [src_vmware_host]
+
+            cloud_tenant.cloud_networks << [dst_cloud_network]
+          end


I would say instead of doing it this way, you should use FactoryBot.create_list with regular assignment instead of << [stuff].

djberg96 · 2019-09-04T00:46:38Z

@thearifismail You got the failures worked out, nice!

agrare · 2019-09-04T12:19:12Z

spec/models/transformation_mapping_item_spec.rb

+        let(:valid_source) { FactoryBot.create(:transformation_mapping_item, :source => src_lan, :destination => dst_cloud_network, :transformation_mapping_id => ops_mapping.id) }
+        let(:invalid_source) { FactoryBot.build(:transformation_mapping_item, :source => dst_cloud_network, :destination => src_lan, :transformation_mapping_id => ops_mapping.id) }
+
+          before do


Looks like there's an extra indentation here

agrare · 2019-09-04T12:20:44Z

spec/models/transformation_mapping_item_spec.rb

+            src_vmware_host.switches << [src_switch]
+            vmware_cluster.hosts << [src_vmware_host]
+
+            cloud_tenant.cloud_networks << [dst_cloud_network]


Is this any different between contexts? If not you can just do this when you define these up here

Actually the way you have those set up e.g. setting the switch for the lan (FactoryBot.create(:lan, :switch => src_switch)) then I doubt you need to do anything extra.

Yes it is a different context.

agrare

After these comments are addressed you don't need either before do;end block

agrare · 2019-09-04T17:02:07Z

spec/models/transformation_mapping_item_spec.rb

+  # Network Validation
+  # ---------------------------------------------------------------------------
+  context "Network validation" do
+    let(:ems_vmware) { FactoryBot.create(:ems_vmware) }


This is a duplicate

manageiq/spec/models/transformation_mapping_item_spec.rb

Line 2 in 2ae1efb

let(:ems_vmware) { FactoryBot.create(:ems_vmware) }

spec/models/transformation_mapping_item_spec.rb

agrare · 2019-09-04T17:03:26Z

spec/models/transformation_mapping_item_spec.rb

+    let(:vmware_cluster) { FactoryBot.create(:ems_cluster, :ext_management_system => ems_vmware) }
+
+    let(:ems_redhat) { FactoryBot.create(:ems_redhat) }
+    let(:redhat_cluster) { FactoryBot.create(:ems_cluster, :ext_management_system => ems_redhat) }


Same

manageiq/spec/models/transformation_mapping_item_spec.rb

Line 6 in 2ae1efb

let(:redhat_cluster) { FactoryBot.create(:ems_cluster, :ext_management_system => ems_redhat) }

agrare · 2019-09-04T17:03:37Z

spec/models/transformation_mapping_item_spec.rb

+    let(:ems_redhat) { FactoryBot.create(:ems_redhat) }
+    let(:redhat_cluster) { FactoryBot.create(:ems_cluster, :ext_management_system => ems_redhat) }
+
+    let(:ems_ops) { FactoryBot.create(:ems_openstack) }


This could just be

manageiq/spec/models/transformation_mapping_item_spec.rb

Line 8 in 2ae1efb

let(:ems_openstack) { FactoryBot.create(:ems_openstack) }

agrare · 2019-09-04T17:16:36Z

spec/models/transformation_mapping_item_spec.rb

+
+        before do
+          src_switch.lans << [src_lan]
+          src_vmware_host.switches << [src_switch]


If you fix this https://github.com/ManageIQ/manageiq/pull/19220/files#diff-1f06e8e1d35539c0a3b5408fc280788dR126 to set the host properly then this isn't needed.

agrare · 2019-09-04T17:18:00Z

spec/models/transformation_mapping_item_spec.rb

+          src_vmware_host.switches << [src_switch]
+          vmware_cluster.hosts << [src_vmware_host]
+
+          cloud_tenant.cloud_networks << [dst_cloud_network]


This isn't needed because you set cloud_network.cloud_tenant here https://github.com/ManageIQ/manageiq/pull/19220/files#diff-1f06e8e1d35539c0a3b5408fc280788dR130

agrare · 2019-09-04T17:18:28Z

spec/models/transformation_mapping_item_spec.rb

+
+          before do
+            src_switch.lans << [src_lan]
+            src_vmware_host.switches << [src_switch]


Similarly if you fix the switch.hosts then you don't need this

agrare · 2019-09-04T17:19:24Z

spec/models/transformation_mapping_item_spec.rb

+
+      context "destination red hat" do
+        let(:dst_rh_host) { FactoryBot.create(:host_redhat, :ems_cluster => redhat_cluster) }
+        let(:dst_rh_switch) { FactoryBot.create(:switch, :host => dst_rh_host) }


This has to be :hosts => [dst_rh_host]

agrare · 2019-09-04T17:19:49Z

spec/models/transformation_mapping_item_spec.rb

+            vmware_cluster.hosts << [src_vmware_host]
+
+            dst_rh_switch.lans << [dst_rh_lan]
+            dst_rh_host.switches << [dst_rh_switch]


Not needed if you set switch.hosts properly

miq-bot · 2019-09-04T19:36:06Z

Checked commits thearifismail/manageiq@d4bcf52~...d27a818 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

spec/models/transformation_mapping_spec.rb

❗ - Line 36, Col 9 - Layout/AlignArguments - Align the arguments of a method call if they span more than one line.

thearifismail · 2019-09-04T22:52:17Z

@agrare made all the changes pointed out.

agrare

There's a let(:ems_openstack) { FactoryBot.create(:ems_openstack) } at the top level context, and two let(:ems_ops) { FactoryBot.create(:ems_openstack) } that could just use the top level one, but this can be done in a follow-up

[V2V] Lan validation in Transformation Mapping

[V2V] Lan validation in Transformation Mapping (cherry picked from commit b701622) https://bugzilla.redhat.com/show_bug.cgi?id=1768517

simaishi · 2019-11-04T17:57:35Z

Ivanchuk backport details:

$ git log -1
commit 5e5246b9eef2861d58714d4d281fcf761138b7c5
Author: Adam Grare <[email protected]>
Date:   Thu Sep 5 07:58:32 2019 -0400

    Merge pull request #19220 from thearifismail/new_lanv
    
    [V2V] Lan validation in Transformation Mapping
    
    (cherry picked from commit b701622b83c45fe6568f48431396d8318f29faec)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1768517

added lan validation to the transformation mapping items model and sp…

d4bcf52

…ecs for it

miq-bot added hammer/yes v2v labels Aug 28, 2019

agrare self-assigned this Aug 28, 2019

agrare requested review from djberg96 and agrare August 28, 2019 19:17

thearifismail mentioned this pull request Aug 28, 2019

[V2V] Datastore validation in Transformation Mapping #19204

Merged

djberg96 reviewed Sep 3, 2019

View reviewed changes

Upaded lan configuration to use switches

7f17f59

agrare reviewed Sep 4, 2019

View reviewed changes

fixed tabs for before block

2ae1efb

agrare requested changes Sep 4, 2019

View reviewed changes

removed duplicate variables and before do end blocks

d27a818

agrare approved these changes Sep 5, 2019

View reviewed changes

agrare added a commit that referenced this pull request Sep 5, 2019

Merge pull request #19220 from thearifismail/new_lanv

b701622

[V2V] Lan validation in Transformation Mapping

agrare merged commit d27a818 into ManageIQ:master Sep 5, 2019

agrare added this to the Sprint 120 Ending Sep 16, 2019 milestone Sep 5, 2019

thearifismail mentioned this pull request Sep 5, 2019

[V2V] Removed a duplicate variable for ems_openstack from transformation_mapping_item_spec. #19263

Merged

agrare mentioned this pull request Nov 1, 2019

[V2V] Refactor ServiceTemplateTransformationPlanTask specs. #19168

Merged

simaishi pushed a commit that referenced this pull request Nov 4, 2019

Merge pull request #19220 from thearifismail/new_lanv

5e5246b

[V2V] Lan validation in Transformation Mapping (cherry picked from commit b701622) https://bugzilla.redhat.com/show_bug.cgi?id=1768517

simaishi added ivanchuk/backported and removed ivanchuk/yes labels Nov 4, 2019

Fryguy added hammer/yes? and removed hammer/yes labels Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[V2V] Lan validation in Transformation Mapping #19220

[V2V] Lan validation in Transformation Mapping #19220

thearifismail commented Aug 28, 2019 •

edited by simaishi

Loading

thearifismail commented Aug 28, 2019 •

edited

Loading

miq-bot commented Aug 28, 2019

miq-bot commented Aug 28, 2019

thearifismail commented Aug 28, 2019

djberg96 Sep 3, 2019

djberg96 commented Sep 4, 2019

agrare Sep 4, 2019

agrare Sep 4, 2019

agrare Sep 4, 2019

thearifismail Sep 4, 2019

agrare left a comment •

edited

Loading

agrare Sep 4, 2019

thearifismail Sep 4, 2019

agrare Sep 4, 2019

thearifismail Sep 4, 2019

agrare Sep 4, 2019

thearifismail Sep 4, 2019

agrare Sep 4, 2019

thearifismail Sep 4, 2019

agrare Sep 4, 2019

thearifismail Sep 4, 2019

agrare Sep 4, 2019

thearifismail Sep 4, 2019

agrare Sep 4, 2019

thearifismail Sep 4, 2019

agrare Sep 4, 2019

thearifismail Sep 4, 2019

miq-bot commented Sep 4, 2019

thearifismail commented Sep 4, 2019

agrare left a comment

simaishi commented Nov 4, 2019

[V2V] Lan validation in Transformation Mapping #19220

[V2V] Lan validation in Transformation Mapping #19220

Conversation

thearifismail commented Aug 28, 2019 • edited by simaishi Loading

thearifismail commented Aug 28, 2019 • edited Loading

miq-bot commented Aug 28, 2019

miq-bot commented Aug 28, 2019

thearifismail commented Aug 28, 2019

Choose a reason for hiding this comment

djberg96 commented Sep 4, 2019

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

agrare left a comment • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Sep 4, 2019

thearifismail commented Sep 4, 2019

agrare left a comment

Choose a reason for hiding this comment

simaishi commented Nov 4, 2019

thearifismail commented Aug 28, 2019 •

edited by simaishi

Loading

thearifismail commented Aug 28, 2019 •

edited

Loading

agrare left a comment •

edited

Loading