Introduce model changes for v2v #16787
Conversation
|
@miq-bot add_label wip |
|
@miq-bot assign @gmcculloug |
app/models/transformation_mapping.rb
Outdated
|
|
||
| def self.select_mappings_by_sources(sources) | ||
| all.select do |m| | ||
| true if m.mapping_items.where(:source => sources).count == sources.count |
There was a problem hiding this comment.
nitpick: you can drop the if, simply:
all.select do |m|
m.mapping_items.where(:source => sources).count == sources.count
end
app/models/transformation_mapping.rb
Outdated
|
|
||
| def self.select_mappings(vms) | ||
| resources = vms.each_with_object([]) do |vm, array| | ||
| array << vm.ems_cluster |
There was a problem hiding this comment.
I'm not sure I know the schemea for vms, but can't we simply do something like
Cluster.where(vm_ids: vms)
Your code will go over every vm and fetch its cluster and strorage, this can be very slow on a very large number of vms
|
@miq-bot add_label v2v |
|
@jntullo please review |
app/models/transformation_mapping.rb
Outdated
| item_map['sources'].each do |source_id| | ||
| transformation_mapping_items << TransformationMappingItem.new( | ||
| :source_id => source_id, | ||
| :source_type => dst_type.camelize, |
There was a problem hiding this comment.
Instead of camelizing, I think that the API should likely deconstruct the type from the api collection to the actual source_type (class). I suggest this because not all of the source types will align perfectly. I am not sure all of the types a transformation item will allow, but just as an example, providers aligns to ExtManagementSystem. I think it would be best for the API to transform providers to ExtManagementSystem. Then you will not have to do anything with the source_type that you get. Thoughts?
There was a problem hiding this comment.
I am perfectly fine with what you proposed.
bzwei
force-pushed
the
v2v_models
branch
2 times, most recently
from
6436393
to
7e9b2da
Compare
bzwei
changed the title
|
@lfu please review |
bzwei
force-pushed
the
v2v_models
branch
2 times, most recently
from
253410d
to
116aa16
Compare
app/models/transformation_mapping.rb
Outdated
| item_map['sources'].each do |source_id| | ||
| transformation_mapping_items << TransformationMappingItem.new( | ||
| :source_id => source_id, | ||
| :source_type => dst_type, |
There was a problem hiding this comment.
Just had an idea. Instead of using the dst_type, how do you feel about the API sending you a source_type? Something along the lines of:
{"name": "new transformation mapping",
"mappings": {
"clusters": [{"source_type": "EmsCluster", "destination": ":id", "sources": [":id"]}
]}}
We would add this in on the create (instead of just changing cluster to EmsCluster), and would also mean that we would not need to do the reverse on a GET (EmsCluster to cluster)
|
Checked commit bzwei@f60db73 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0 app/models/transformation_mapping_item.rb
|
|
LGTM 👍 |
| @@ -0,0 +1,3 @@ | |||
| FactoryGirl.define do | |||
| factory :transformation_mapping_item | |||
There was a problem hiding this comment.
@bzwei You are using TransformationMappingItem.new in the spec, is this factory needed at this point?
There was a problem hiding this comment.
spec/models/factory_girl_spec.rb requires all models to have a factory defined.
|
@miq-bot add_label enhancement |
|
@miq-bot add_label gaprindashvili/yes Needed by ManageIQ/manageiq-api#313 for gaprindashvili v2v support |
Introduce model changes for v2v (cherry picked from commit a6a868c)
|
Gaprindashvili backport details: |
Initial work for modeling v2v tables introduced in ManageIQ/manageiq-schema#149
Deletion of the mapping with items is automatically supported. Update with single request is to be done in a future PR.