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.

Sign up for GitHub

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b07d5c6
Explicitly pass transformation_mapping when generating mapping items.
Aug 27, 2019
bcd4777
Replace TransformationMapping.new with TransformationMapping.create, …
Aug 27, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions app/controllers/api/transformation_mappings_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,8 @@ module Api
class TransformationMappingsController < BaseController
def create_resource(_type, _id, data = {})
raise "Must specify transformation_mapping_items" unless data["transformation_mapping_items"]
TransformationMapping.new(data.except("transformation_mapping_items")).tap do |mapping|
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 •
edited
Loading

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 •
edited
Loading

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.

end
rescue StandardError => err
raise BadRequestError, "Could not create Transformation Mapping - #{err}"
Expand All @@ -17,7 +16,7 @@ def edit_resource(type, id, data)
updated_data = data.except("transformation_mapping_items")
transformation_mapping.update_attributes!(updated_data) if updated_data.present?

transformation_mapping.transformation_mapping_items = create_mapping_items(data["transformation_mapping_items"])
transformation_mapping.transformation_mapping_items = create_mapping_items(data["transformation_mapping_items"], transformation_mapping)
transformation_mapping.save!
transformation_mapping
rescue StandardError => err
Expand Down Expand Up @@ -51,7 +50,7 @@ def vm_flavor_fit_resource(_type, _id, data)

def add_mapping_item_resource(type, id, data)
resource_search(id, type, collection_class(type)).tap do |mapping|
mapping.transformation_mapping_items.append(create_mapping_items([data]))
mapping.transformation_mapping_items.append(create_mapping_items([data], mapping))
mapping.save!
end
rescue StandardError => err
Expand All @@ -60,10 +59,14 @@ def add_mapping_item_resource(type, id, data)

private

def create_mapping_items(items)
def create_mapping_items(items, mapping)
items.collect do |item|
raise "Must specify source and destination hrefs" unless item["source"] && item["destination"]
TransformationMappingItem.new(:source => fetch_mapping_resource(item["source"]), :destination => fetch_mapping_resource(item["destination"]))
TransformationMappingItem.new(
:transformation_mapping => mapping,
:source => fetch_mapping_resource(item["source"]),
:destination => fetch_mapping_resource(item["destination"])
)
end
end

Expand Down