Support publish VM #95
Conversation
masayag
force-pushed
the
support_publish_vm
branch
3 times, most recently
from
49ace61
to
ef1e368
Compare
masayag
force-pushed
the
support_publish_vm
branch
2 times, most recently
from
c9ed41a
to
a3ce1b4
Compare
| source.with_provider_connection(VERSION_HASH) do |connection| | ||
| template = template_service_by_href(phase_context[:new_vm_ems_ref], connection).get | ||
| status = template.status | ||
| logger.info("The Template being cloned is #{status}") |
There was a problem hiding this comment.
Shouldn't it be "Cloned template status is #{status}"?
There was a problem hiding this comment.
The supported values are: ok, illegal, locked, so they fit for the message format, however, I"ll change it to better fit.
| @@ -437,6 +490,10 @@ def cluster_from_href(href, connection) | |||
| connection.system_service.clusters_service.cluster_service(uuid_from_href(href)).get | |||
| end | |||
|
|
|||
| def storage_from_href(href, connection) | |||
There was a problem hiding this comment.
We already fetch storage domain in different place. I think that we start to have similar responsibilities in inventory and ovirt_services. In my opinion there is no need to have two distinct object having the same responsibility.
There was a problem hiding this comment.
we'd might access RHV from different places of the provider. We can think of introducing a class that will be responsible for interacting between the provider to RHV via v4, I'm not sure this is the PR to introduce such.
We should first get into agreement if we'd prefer that method over letting each workflow/scenario interact independently using the engine's connection.
There was a problem hiding this comment.
I am not saying that we should fix it in this PR. I am saying that we already have similar code in different place. I think it would be better to define responsibilities of the classes we use to talk to the engine.
There was a problem hiding this comment.
We'll track refactoring of ovirt_services and querying of engine's request by issue #100
| @@ -476,6 +533,11 @@ def vm_service_by_href(href, connection) | |||
| connection.system_service.vms_service.vm_service(vm_uuid) | |||
| end | |||
|
|
|||
| def template_service_by_href(href, connection) | |||
|
|
||
| def prepare_clone_options | ||
| if source.template? | ||
| vm_clone_options |
There was a problem hiding this comment.
Please add some comment here. I was puzzled by this code because I see is_templare => true then do vm. I noticed similar code in line 80 where comment helped.
There was a problem hiding this comment.
agreed. I've start looking into the 'clone vm' workflow and encountered that using the source.template? will have to be replaced by a different method to identify the existing flow.
With the current PR, we're reusing the same workflow entity for add vm and add template. However, in 'clone vm' both source and destination are vms. I'll handle it as part of 'clone vm' PR.
I'll add comment here as well.
| _log.info("Destination image locked; re-queuing") | ||
| requeue_phase | ||
| else | ||
| message = "Starting New #{destination_type} Customization" |
There was a problem hiding this comment.
Why we use capital letter for "new" and "customization"?
There was a problem hiding this comment.
developer excuses #211: "it was like that before"
seems like other providers (vmware and microsoft) have the exact message (with the capitals)
| return nil unless seal | ||
|
|
||
| if get_source_vm.platform == 'windows' | ||
| _("Template Sealing is supported only for non-Windows OS.") |
There was a problem hiding this comment.
Is there a reason to have "sealing" starting with capital letter?
Creating a template from a VM is supported by RHV since forever. The PR introduces a new dialog to allow the creation of a template from a VM, including the ability to seal a template (for non-windows vms). https://bugzilla.redhat.com/show_bug.cgi?id=1373076
masayag
force-pushed
the
support_publish_vm
branch
from
a3ce1b4
to
87e61e7
Compare
|
Checked commit masayag@87e61e7 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@masayag code climate noticed duplication. It seems not that hard to have one method. What do you think? |
|
Based on my conversation with @masayag we will ignore codeclimate for now. |
Creating a template from a VM is supported by RHV since forever.
The PR introduces a new dialog to allow the creation of a template from
a VM, including the ability to seal a template (for non-windows vms).
https://bugzilla.redhat.com/show_bug.cgi?id=1373076
Screenshots of the new dialog:
Request tab:
Purpose tab:
Catalog tab:
Environment tab:
Schedule tab:
Due to a limitation of RHV, when specifying a storage domain, all of the template's disks will be created on the specified storage domain (while on RHV the user can specify storage domain per disk)