rhv: removed the option to migrate the VMs outside of the cluster. #84
Conversation
The support for cross cluster migrations was added to oVirt only as a workaround for el6->el7 migrations but should not be exposed. Since the current code in manageiq anyway did not work properly, removing the support for it completely - it is a low level functionality, it is obsoleted and discouraged to be used. Links https://bugzilla.redhat.com/show_bug.cgi?id=1398287
|
Checked commit jelkosz@5c87f1d with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 **
|
|
The fronted part. The backend part is ManageIQ/manageiq#13077 |
|
@borod108 Hi, not sure why the assign did not work, but could you please have a look at this? It is the same patch you have already reviewed but had to be split after the repo split. |
There was a problem hiding this comment.
I have to say that "typ" variable and writing "migrate" == typ instead of type == "migrate" is making me uncomfortable but its is only a matter of style and I do not have any right to insist on that :) I think its good to go.
Only thing is there is a weird rubocop comment, I think its probably a false positive, but I would check everything works on the UI side before merging just in case...
| @@ -1797,6 +1797,15 @@ def task_supported?(typ) | |||
| end | |||
|
|
|||
| vms = VmOrTemplate.where(:id => vm_ids) | |||
| if "migrate" == typ | |||
| if vms.map(&:ext_management_system).uniq.compact.any? do |ems| | |||
| ems.respond_to?("supports_#{typ}_for_all?") && !ems.send("supports_#{typ}_for_all?", vms) | |||
There was a problem hiding this comment.
I'd just remove the string interpolation here, which would make it easier to read too
There was a problem hiding this comment.
please see my comment on the other PR ManageIQ/manageiq#13077 (comment)
So, not sure about this.
There was a problem hiding this comment.
Yes, please don't do that. No more magical send with string interpolation, and especially not when you know the type is migrate :).
https://en.wikipedia.org/wiki/Yoda_conditions But I feel you pain :) |
| :dialog => dialog, | ||
| :label => title_for_cluster, | ||
| :keys => keys}) | ||
| - if wf.field_supported(:cluster) |
There was a problem hiding this comment.
is this set somewhere? I dont see it in this PR
There was a problem hiding this comment.
@durandom what are you referring to?
| @@ -1797,6 +1797,15 @@ def task_supported?(typ) | |||
| end | |||
|
|
|||
| vms = VmOrTemplate.where(:id => vm_ids) | |||
| if "migrate" == typ | |||
| if vms.map(&:ext_management_system).uniq.compact.any? do |ems| | |||
| ems.respond_to?("supports_#{typ}_for_all?") && !ems.send("supports_#{typ}_for_all?", vms) | |||
There was a problem hiding this comment.
please see my comment on the other PR ManageIQ/manageiq#13077 (comment)
So, not sure about this.
| @@ -1797,6 +1797,15 @@ def task_supported?(typ) | |||
| end | |||
|
|
|||
| vms = VmOrTemplate.where(:id => vm_ids) | |||
| if "migrate" == typ | |||
There was a problem hiding this comment.
No Yoda conditions please :) How we do things this is not.
|
Replaced by #207, closing. |
The support for cross cluster migrations was added to oVirt only as a
workaround for el6->el7 migrations but should not be exposed. Since the current
code in manageiq anyway did not work properly, removing the support for it
completely - it is a low level functionality, it is obsoleted and discouraged
to be used.
Links
https://bugzilla.redhat.com/show_bug.cgi?id=1398287