Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly pass transformation_mapping when generating mapping items #664

Closed
wants to merge 2 commits into from
Closed

Explicitly pass transformation_mapping when generating mapping items #664

wants to merge 2 commits into from

Conversation

djberg96
Copy link
Contributor

In order to properly perform validations on a TransformationMappingItem, we need the parent to already be defined in some cases. This modifies the create_mapping method so that it accepts and uses the parent transformation mapping when creating a new transformation mapping item.

Tied to ManageIQ/manageiq#19204

@djberg96
Copy link
Contributor Author

@miq-bot add_label transformation

@miq-bot miq-bot added the v2v label Aug 27, 2019
@thearifismail
Copy link
Contributor

change new to create on line 5 TransformationMapping.new(data.except("transformation_mapping_items")).tap do |mapping| to create a transformation map with a valid ID, which is needed for TransformationMappingItems.

@miq-bot
Copy link
Member

miq-bot commented Aug 27, 2019

Checked commits https://github.com/djberg96/manageiq-api/compare/b07d5c6b53e460c3670a092d84461a342cdeb512~...bcd4777c151a119809c79e206d1f7be390868d52 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

mapping.transformation_mapping_items = create_mapping_items(data["transformation_mapping_items"])
mapping.save!
TransformationMapping.create(data.except("transformation_mapping_items")).tap do |mapping|
mapping.transformation_mapping_items = create_mapping_items(data["transformation_mapping_items"], mapping)
Copy link
Member

@agrare agrare Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if you do:

mapping = TransformationMapping.new(data.except("transformation_mapping_items"))
mapping.transformation_mapping_items = create_mapping_items(data["transformation_mapping_items"], mapping)
mapping.save!

Then if the transformation_mapping_item validation fails it will rollback the create of the top-level transformation_mapping which is what I believe @thearifismail was going for in ManageIQ/manageiq#19216 wrong link sorry, here it is ManageIQ/manageiq#19204

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you if that is the behavior you want though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but the mapping items still wouldn't have a parent at that point, which we need to get at datastore and lan information.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thearifismail, at least, I thought that was the issue, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, but the mapping items still wouldn't have a parent at that point, which we need to get at datastore and lan information.

If you pass the mapping in to TransformationMappingItem.new they will though, see ManageIQ/manageiq#19204 (comment) I tested that case where the parent TM isn't saved when the TMI validations are run

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@djberg96 is right. We had to use create to get transformation_mapping_id generated for use with the subsequent mapping items calls. This is what I have:

      TransformationMapping.create(data.except("transformation_mapping_items")).tap do |mapping|
        mapping.transformation_mapping_items = create_mapping_items(data["transformation_mapping_items"], mapping)
        mapping.save!
      end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try it though?

>> vc = ManageIQ::Providers::Vmware::InfraManager.first; rhv = ManageIQ::Providers::Redhat::InfraManager.first
>> source_storage = Storage.create!(:name => "unattached storage")
>> tm = TransformationMapping.new(:name => "my transformation")
>> tm.transformation_mapping_items = [
  TransformationMappingItem.new(:source => vc.ems_clusters.first, :destination => rhv.ems_clusters.first, :transformation_mapping => tm),
  TransformationMappingItem.new(:source => vc.lans.first, :destination => rhv.lans.first, :transformation_mapping => tm),
  TransformationMappingItem.new(:source => source_storage, :destination => rhv.storages.first, :transformation_mapping => tm)
]
>> tm.save!
   (0.3ms)  BEGIN
   (0.2ms)  ROLLBACK
ActiveRecord::RecordInvalid (Validation failed: TransformationMapping: Transformation mapping items is invalid)

And just to make sure:

   31:   def validate_source_datastore
   32:     byebug
   33:     tm                   = transformation_mapping
=> 34:     tmis                 = tm.transformation_mapping_items.where(:source_type => "EmsCluster")
   35:     src_cluster_storages = tmis.collect(&:source).flat_map(&:storages)
   36:     source_storage       = source
   37: 
   38:     unless src_cluster_storages.include?(source_storage)
(byebug) tm
#<TransformationMapping id: nil, name: "my transformation", description: nil, comments: nil, state: nil, options: {}, tenant_id: nil, created_at: nil, updated_at: nil>

and

   43:   def validate_destination_datastore
   44:     byebug
   45:     tm                  = transformation_mapping
=> 46:     destination_storage = destination
   47: 
   48:     if destination.kind_of?(Storage) # red hat
   49:       tmis                 = tm.transformation_mapping_items.where(:destination_type=> "EmsCluster")
   50:       dst_cluster_storages = tmis.collect(&:destination).flat_map(&:storages)
(byebug) tm
#<TransformationMapping id: nil, name: "my transformation", description: nil, comments: nil, state: nil, options: {}, tenant_id: nil, created_at: nil, updated_at: nil>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should note that

>> tm.transformation_mapping_items = [
  TransformationMappingItem.new(:source => vc.ems_clusters.first, :destination => rhv.ems_clusters.first, :transformation_mapping => tm),
  TransformationMappingItem.new(:source => vc.lans.first, :destination => rhv.lans.first, :transformation_mapping => tm),
  TransformationMappingItem.new(:source => source_storage, :destination => rhv.storages.first, :transformation_mapping => tm)
]

Isn't how I'd do it, but it is functionally equivalent to what is being done here.

Copy link
Contributor

@thearifismail thearifismail Aug 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had started off using new but it would not generate any mapping id and had observed exactly what @agrare observed using byebug, which produced transformation_mapping_id = nil. That's why we had to fall back to using create.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait I'm confused, I was saying that worked perfectly. Not having an :id yet isn't an issue, you can run your validations and if they pass tm.save! will commit the whole set to the database.

@djberg96
Copy link
Contributor Author

Closed in favor of #666

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants