Adjust VM validity correctly while editing a ServiceTemplate record #18065
Conversation
VMs belonging to a Plan are valid VMs and should not be categorized as `in_other_plan`
validate_vms_resource to include service template_id
ManageIQ/manageiq-api#486
| @@ -31,6 +31,7 @@ | |||
|
|
|||
| describe '#search_vms_and_validate' do | |||
| let(:vm) { FactoryGirl.create(:vm_vmware, :name => 'test_vm', :ems_cluster => src, :ext_management_system => FactoryGirl.create(:ext_management_system)) } | |||
| let(:vm2) { FactoryGirl.create(:vm_vmware, :name => 'test_vm2', :ems_cluster => src, :ext_management_system => FactoryGirl.create(:ext_management_system)) } | |||
There was a problem hiding this comment.
Please remove :name attribute since the factory sequences this for you.
There was a problem hiding this comment.
Thanks, addressed that.
| @@ -98,7 +99,7 @@ def vm_migration_status(vm) | |||
| return VM_MIGRATED unless vm_as_resources.where(:status => ServiceResource::STATUS_COMPLETED).empty? | |||
|
|
|||
| # VM failed in previous migration | |||
| vm_as_resources.all? { |rsc| rsc.status == ServiceResource::STATUS_FAILED } ? VM_VALID : VM_IN_OTHER_PLAN | |||
| vm_as_resources.all? { |rsc| rsc.status == ServiceResource::STATUS_FAILED || rsc.service_template_id.to_s == @service_template_id } ? VM_VALID : VM_IN_OTHER_PLAN | |||
There was a problem hiding this comment.
Why is @service_template_id a string?
There was a problem hiding this comment.
The UI has service_template_id info available to it as a string (extracted from a JSON), so it sends that as is to the backend.
Usually the interaction between the UI and the backend when it comes to id's is always in the string format.
There was a problem hiding this comment.
I know about the JSON limitation with bigint, but once we're in a model, I would expect anything_id to be a bigint.
There was a problem hiding this comment.
Ok, in that case I would have to apply .to_i to service_template_id in
Does that work?
|
LGTM 👍 |
AparnaKarve
force-pushed
the
adjust_vm_validity_edit_plan
branch
from
a874eb5
to
c3a331d
Compare
| @mapping = mapping | ||
| @vm_list = vm_list | ||
| @service_template_id = service_template_id.to_i |
There was a problem hiding this comment.
This can hit a null pointer if the param is nil. Probably just need to protect against this, or decide that the service_template_id param can't be nil.
There was a problem hiding this comment.
Ok, I addressed that with -
@service_template_id = service_template_id.try(:to_i)There was a problem hiding this comment.
Interestingly enough, nil.to_i returns 0 :)
AparnaKarve
force-pushed
the
adjust_vm_validity_edit_plan
branch
from
c7dc8ae
to
d41949d
Compare
|
Checked commits AparnaKarve/manageiq@36de171~...d41949d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 app/models/transformation_mapping/vm_migration_validator.rb
|
Adjust VM validity correctly while editing a ServiceTemplate record (cherry picked from commit 9caf12f)
|
Hammer backport details: |
Currently VM belonging to a migration plan is being categorized as an invalid VM (
in_other_plan) when the plan is being edited.This is incorrect since the VM is already part of the plan and should be evaluated to a valid VM.
Fixes ManageIQ/manageiq-v2v#677