Save host for a Vm after migration #13511
Conversation
|
@masayag please review |
|
@miq-bot add-label euwe/yes |
borod108
force-pushed
the
bugs/1368925migrate_vm
branch
from
84a92fc to
eb093f7
Compare
| @@ -18,6 +20,11 @@ def self.ems_inv_to_hashes(inv) | |||
| # Clean up the temporary cluster-datacenter references | |||
| result[:clusters].each { |c| c.delete(:datacenter_id) } | |||
|
|
|||
| added_hosts_from_vms.each do |h| | |||
| result[:hosts] = result[:hosts] << h | |||
There was a problem hiding this comment.
no need to assign the updated collection to itself.
There was a problem hiding this comment.
Besides the spec question it looks good to me.
Although I can map this to the attached BZ.
The BZ is talking about Hosts being deleted still showing up.
The code reads like you add hosts that would have been missed otherwise.
Maybe you can update the PR description to explain a bit more what this should do?
| @@ -66,6 +66,32 @@ | |||
| assert_storage(storage, vm) | |||
| end | |||
|
|
|||
| context 'vm migration' do | |||
| before(:each) do | |||
There was a problem hiding this comment.
a similar before block is at the beginning of this spec. Do you really need both?
How about moving the reset of this file into its own context with individual before blocks.
Or even better get rid of them, as they tend to be overseen in larger files
There was a problem hiding this comment.
I had to make a new vcr recording, I do not know how to get rid of this block. Do I understand correctly and you want me to move this context into its own spec file? just seemed to me this is the right logical place for it...
There was a problem hiding this comment.
Sure I can move the ems details into "let" and update those in the context, would that be better?
There was a problem hiding this comment.
move this context into its own spec file
No, I meant wrapping the other tests in this file in their own context with their own before block and leave this test as is.
Now this before block gets executed and the one at the beginning of the file. And I think this is not what you want.
There was a problem hiding this comment.
No, I meant wrapping the other tests in this file in their own context with their own before block and leave this test as is.
Now this before block gets executed and the one at the beginning of the file. And I think this is not what you want.
Thats still one context within another context, right? So still both before blocks get executed...
There was a problem hiding this comment.
As far as I can see, there are now two contexts each with its own before(:each) block like you requested, am I missing something?
borod108
force-pushed
the
bugs/1368925migrate_vm
branch
2 times, most recently
from
9ed9922 to
ad2f2d1
Compare
Fix saving the host of a Vm after it was migrated when doing targeted refresh of the Vm. Before if the hosts inventory was not fetched sperately by the refresh, the host of a VM was not updated properly even though its uid was returned as part of the target Vm inventory. Now we take the information from the Vm inventory and use it to update the Vms host properly. https://bugzilla.redhat.com/show_bug.cgi?id=1368925
borod108
force-pushed
the
bugs/1368925migrate_vm
branch
from
ad2f2d1 to
e346b46
Compare
|
Indeed, I changed both the PR description and the bug title. |
|
Checked commit borod108@e346b46 with ruby 2.2.6, rubocop 0.46.0, and haml-lint 0.19.0 |
|
That's a pretty cool solution @borod108 :) let me try to think if there is anything wrong with it 😉 |
|
Not quite as clean as providing the host in inventory, but lets you pass just the ems_ref and uid_ems without having to rework the host_inv_to_hashes so 👍 from me |
|
@borod108 Can't do a clean cherry-pick due to refactoring. Please make Euwe-specific PR (referencing this one) or suggest other PRs to backport. |
|
Backported to Euwe via #13618 |
Fix saving the host of a Vm after it was migrated when doing
targeted refresh of the Vm.
https://bugzilla.redhat.com/show_bug.cgi?id=1368925