Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow vApp customization prior provisioning #185

Merged
merged 1 commit into from
Feb 28, 2018

Conversation

miha-plesko
Copy link
Contributor

@miha-plesko miha-plesko commented Feb 19, 2018

vApp provisioning is already possible via service dialogs, but only a very modest customization is allowed. With this commit we implement support for much more enhanced vApp customization by allowing modification of following parameters:

  • vApp name, tenant, availability zone, deploy?, power_on?
  • vApp network customization (parent, fence mode, gateway, netmask, dns1, dns2)
  • VM customization: name, hostname, CPU, MEM, disk size, NIC configuration (connected?, network, ip mode, ip address)

Below please find screenshot of the new Service Dialog:

capture

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1550909

/cc @gberginc

@miha-plesko
Copy link
Contributor Author

I've marked this WIP because it depends on more recent version of fog-vcloud-director gem being released, other than that the PR should be ready for review. @gberginc may I kindly ask you for first pass?

@gberginc
Copy link
Contributor

@miha-plesko I am working through this PR. One quick comment I have: I suggest we split the VM customisations to additional tabs, i.e. we have the Basic info tab with Options, vApp Parameters and vApp Networks and the for each VM in the template a separate tab with VM-specific options. This way we will also be able to group the VM parameters better (e.g. all network related opts can go in a separate group).

Copy link
Contributor

@gberginc gberginc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miha-plesko this is one hell of an extension to the existing vapp provisioning. I've got some comments and questions, but in general I see that specs cover the code quite nicely.

As discussed, please record a video for a complex vApp template that shows how customisations are propagated to VCD.

:fenceMode => "bridged"
src_vm = {
:vm_id => "vm-#{vm_id}",
:guest_customization => { :ComputerName => vm_opts['hostname'] },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the user does not provide the hostname? Should this be protected like the src_vm[:name] in L34? The question here is what happens in the vcloud backend?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instatiation request is refused with A computer name must be specified. if nil or empty string is passed, let me update fog to automatically exclude . Fixed as suggested, thx.

vm_params[vm_id] ||= {}
# Store the parameter value.
vm_params[vm_id][param] = value
key = Base64.decode64(param_match.captures.first)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll need to understand why this encoding is required a bit more. Is this something that cannot be solved by some IDs or indices?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment below. I think introducing indeces would complicate things.

@@ -37,35 +40,240 @@ def vapp_parameters
]
end

def vm_xpaths(key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reference you could point to with these specific ResourceTypes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, added two links.

cores_per_socket = Integer(el.elements[vm_xpaths(:cores_per_socket)]&.text || num_cores)
memory_mb = Integer(el.elements[vm_xpaths(:memory_mb)]&.text || 1024)
hostname = el.elements[vm_xpaths(:hostname)]&.text
vapp_networks = ovf.find_match("//vcloud:NetworkConfig[not(@networkName = 'none')]/@networkName").map(&:value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to move all XPaths to the helper func?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done.

vm_name = el.elements["vcloud:ComputerName"].text
ovf.each_element("//ovf:VirtualSystem") do |el|
vm_id = el.elements[vm_xpaths(:id)].text
vm_name = el.elements[vm_xpaths(:name)]&.text || 'Virtual Machine'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you intentionally changing the vm_name here? Previously it was ComputerName from GuestCustomizationSection, but now it's the ovf:Name (or Virtual Machine by default).

As you are using the ComputerName below, I wonder if the default might rather be el.elements[vm_xpaths(:hostname)]&.text as well (instead of Virtual Machine).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there are two different things here: VM name and VM hostname. The most notable difference is that VM name can contain spaces while hostname is quite more limited:

capture

I think we were previously extracting wrong values from the XML (hostname instead name) and that it's correct now. But I agree with your suggestion to default to hostname in case when name is not provided.

"#{param}-#{group_name}___#{extra_attrs}"
end

def insert_vapp_networks(ovf)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you altering the OVF here? Adding some defaults for simpler parsing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. At the moment we don't support dynamical additon of vapp networks (i.e. we don't have the + button on the form) therefore user will be happy to always see at least one vApp network there to connect to. Imagine situation where all VMs are disconnected in vapp template -> then user couldn't connect anywhere using Service Dialog (but with approach we've taken, user is now always able at least to connect to "Default Network" and she can connect this Default Network to any VDC network avaialbe).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok; I understand now the problem, thanks for the clarification.

For me it is important that this can be overcome with a proper vApp template specification.

:label => "Gateway",
:data_type => "string",
:default_value => gateway,
:constraints => []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be beneficial if all IP fields would have a OrchestrationParameterPattern constraint.

Example regexp: ^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?).){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$ from SO: http://www.rubular.com/r/M6FsqOfUcO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks, done.


def param_name(param, group_name, extra = {})
group_name = Base64.encode64(group_name).strip
extra_attrs = extra.present? ? Base64.encode64(extra.to_yaml).strip : ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned before: the reason for base64 encoding should be elaborated in the code. From our discussions it seems like this is basically preparation for even more complex parameters. Because this is all automatically generated, I am fine with these kind of names, unless others have better proposals.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think introducing indeces would complicate things and make them harder to understand. For now I've simply added this description to the code:

# Compose id for HTML form element. Bear in mind that we will have only two values available later
# when parsing the page: form element id and form element value. Value is what user provides (e.g. disk size in
# megabytes). Based on id we need to be able to tell following (two-level categorization):
# - what VM does disk belong to
# - what is ID of disk
# Even more complex example is with vapp network customization. For given IP range start address value we
# need to be able to tell following (three-level categorization):
# - what is the IP range index (can be many of them in same IP scope)
# - what is the IP scope index (can be many of them in same vApp network)
# - what is the vApp network name
# This function encodes arbitrary number of parameters in a valid HTML id value. It produces following
# output:
#   {PARAM_NAME}-{TOP_CATEGORY}___{ARBITRARY_HASH}
# Example for disk size:
#   disk_mb-Base64('vm-id-123')___Base64({ :disk_id => '2000' }.to_yaml)
# Example for IP range start address:
#   ip_range_start-Base64('vApp Network Name')__Base64({ :ip_rage_idx => 0, :id_scope_idx => 0 })
#
# NOTE: The TOP_CATEGORY and ARBITRARY_HASH parameters are base64 encoded to make sure HTML id value will be valid
# (must not contain spaces).

Copy link
Contributor Author

@miha-plesko miha-plesko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @gberginc for review, I've applied all your comments, but I havent updated the failing spec yet. Please see.

:fenceMode => "bridged"
src_vm = {
:vm_id => "vm-#{vm_id}",
:guest_customization => { :ComputerName => vm_opts['hostname'] },
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instatiation request is refused with A computer name must be specified. if nil or empty string is passed, let me update fog to automatically exclude . Fixed as suggested, thx.

@@ -37,35 +40,240 @@ def vapp_parameters
]
end

def vm_xpaths(key)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, added two links.

vm_name = el.elements["vcloud:ComputerName"].text
ovf.each_element("//ovf:VirtualSystem") do |el|
vm_id = el.elements[vm_xpaths(:id)].text
vm_name = el.elements[vm_xpaths(:name)]&.text || 'Virtual Machine'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there are two different things here: VM name and VM hostname. The most notable difference is that VM name can contain spaces while hostname is quite more limited:

capture

I think we were previously extracting wrong values from the XML (hostname instead name) and that it's correct now. But I agree with your suggestion to default to hostname in case when name is not provided.

cores_per_socket = Integer(el.elements[vm_xpaths(:cores_per_socket)]&.text || num_cores)
memory_mb = Integer(el.elements[vm_xpaths(:memory_mb)]&.text || 1024)
hostname = el.elements[vm_xpaths(:hostname)]&.text
vapp_networks = ovf.find_match("//vcloud:NetworkConfig[not(@networkName = 'none')]/@networkName").map(&:value)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done.

:label => "Gateway",
:data_type => "string",
:default_value => gateway,
:constraints => []
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks, done.


def param_name(param, group_name, extra = {})
group_name = Base64.encode64(group_name).strip
extra_attrs = extra.present? ? Base64.encode64(extra.to_yaml).strip : ''
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think introducing indeces would complicate things and make them harder to understand. For now I've simply added this description to the code:

# Compose id for HTML form element. Bear in mind that we will have only two values available later
# when parsing the page: form element id and form element value. Value is what user provides (e.g. disk size in
# megabytes). Based on id we need to be able to tell following (two-level categorization):
# - what VM does disk belong to
# - what is ID of disk
# Even more complex example is with vapp network customization. For given IP range start address value we
# need to be able to tell following (three-level categorization):
# - what is the IP range index (can be many of them in same IP scope)
# - what is the IP scope index (can be many of them in same vApp network)
# - what is the vApp network name
# This function encodes arbitrary number of parameters in a valid HTML id value. It produces following
# output:
#   {PARAM_NAME}-{TOP_CATEGORY}___{ARBITRARY_HASH}
# Example for disk size:
#   disk_mb-Base64('vm-id-123')___Base64({ :disk_id => '2000' }.to_yaml)
# Example for IP range start address:
#   ip_range_start-Base64('vApp Network Name')__Base64({ :ip_rage_idx => 0, :id_scope_idx => 0 })
#
# NOTE: The TOP_CATEGORY and ARBITRARY_HASH parameters are base64 encoded to make sure HTML id value will be valid
# (must not contain spaces).

"#{param}-#{group_name}___#{extra_attrs}"
end

def insert_vapp_networks(ovf)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. At the moment we don't support dynamical additon of vapp networks (i.e. we don't have the + button on the form) therefore user will be happy to always see at least one vApp network there to connect to. Imagine situation where all VMs are disconnected in vapp template -> then user couldn't connect anywhere using Service Dialog (but with approach we've taken, user is now always able at least to connect to "Default Network" and she can connect this Default Network to any VDC network avaialbe).

vm_params[vm_id] ||= {}
# Store the parameter value.
vm_params[vm_id][param] = value
key = Base64.decode64(param_match.captures.first)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment below. I think introducing indeces would complicate things.

@gberginc
Copy link
Contributor

@miha-plesko As far as I am concerned, this PR does what it is supposed to do. Please update the spec to check the fields generated by the converter to allow @agrare to review.

@agrare agrare self-assigned this Feb 23, 2018
@agrare
Copy link
Member

agrare commented Feb 23, 2018

Wow a lot of good stuff here @miha-plesko :)


def collect_stack_parameters(allowed)
separator = '___'
stack_parameters.each_with_object({}) do |(key, value), params|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miha-plesko you might have already but can you paste an example of what stack_parameters looks like?

I think we could do this with pre-parsing and hashes instead of regex, base64, and yaml but you might have already tried that

Copy link
Contributor Author

@miha-plesko miha-plesko Feb 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here goes the stack_parameters:

{
 "deploy"=>"f",
 "powerOn"=>"f",
 "parent-UmVkSGF0IFByaXZhdGUgbmV0d29yayA0Mw==___"=>"b915be99-1471-4e51-bcde-da2da791b98f",
 "fence_mode-UmVkSGF0IFByaXZhdGUgbmV0d29yayA0Mw==___"=>"bridged",
 "gateway-UmVkSGF0IFByaXZhdGUgbmV0d29yayA0Mw==___LS0tCjppcF9zY29wZV9pZHg6IDAK"=>"192.168.43.1",
 "netmask-UmVkSGF0IFByaXZhdGUgbmV0d29yayA0Mw==___LS0tCjppcF9zY29wZV9pZHg6IDAK"=>"255.255.255.0",
 "dns1-UmVkSGF0IFByaXZhdGUgbmV0d29yayA0Mw==___LS0tCjppcF9zY29wZV9pZHg6IDAK"=>"192.168.43.1",
 "dns2-UmVkSGF0IFByaXZhdGUgbmV0d29yayA0Mw==___LS0tCjppcF9zY29wZV9pZHg6IDAK"=>nil,
 "parent-RGVmYXVsdCBOZXR3b3Jr___"=>nil,
 "fence_mode-RGVmYXVsdCBOZXR3b3Jr___"=>"isolated",
 "gateway-RGVmYXVsdCBOZXR3b3Jr___LS0tCjppcF9zY29wZV9pZHg6IDAK"=>"192.168.2.1",
 "netmask-RGVmYXVsdCBOZXR3b3Jr___LS0tCjppcF9zY29wZV9pZHg6IDAK"=>"255.255.255.0",
 "dns1-RGVmYXVsdCBOZXR3b3Jr___LS0tCjppcF9zY29wZV9pZHg6IDAK"=>"8.8.8.8",
 "dns2-RGVmYXVsdCBOZXR3b3Jr___LS0tCjppcF9zY29wZV9pZHg6IDAK"=>nil,
 "instance_name-M2ZiZWI4MDctMGEzMS00YzFmLWEwMDgtOWI4YzI5MDZkNjYw___"=>"Database VM",
 "hostname-M2ZiZWI4MDctMGEzMS00YzFmLWEwMDgtOWI4YzI5MDZkNjYw___"=>"DatabaseVM",
 "num_cores-M2ZiZWI4MDctMGEzMS00YzFmLWEwMDgtOWI4YzI5MDZkNjYw___"=>2,
 "cores_per_socket-M2ZiZWI4MDctMGEzMS00YzFmLWEwMDgtOWI4YzI5MDZkNjYw___"=>2,
 "memory_mb-M2ZiZWI4MDctMGEzMS00YzFmLWEwMDgtOWI4YzI5MDZkNjYw___"=>1024,
 "disk_mb-M2ZiZWI4MDctMGEzMS00YzFmLWEwMDgtOWI4YzI5MDZkNjYw___LS0tCjpkaXNrX2lkOiAnMjAwMCcK"=>4096,
 "nic_network-M2ZiZWI4MDctMGEzMS00YzFmLWEwMDgtOWI4YzI5MDZkNjYw___LS0tCjpuaWNfaWR4OiAnMCcK"=>"Default Network",
 "nic_mode-M2ZiZWI4MDctMGEzMS00YzFmLWEwMDgtOWI4YzI5MDZkNjYw___LS0tCjpuaWNfaWR4OiAnMCcK"=>"MANUAL",
 "nic_ip_address-M2ZiZWI4MDctMGEzMS00YzFmLWEwMDgtOWI4YzI5MDZkNjYw___LS0tCjpuaWNfaWR4OiAnMCcK"=>"192.168.2.100",
 "instance_name-MjVlYmQ3NDctOTI5OS00MjUzLTk0YTItNzQ4Y2QxNjI2OWI3___"=>"Web Server VM",
 "hostname-MjVlYmQ3NDctOTI5OS00MjUzLTk0YTItNzQ4Y2QxNjI2OWI3___"=>"WebServerVM",
 "num_cores-MjVlYmQ3NDctOTI5OS00MjUzLTk0YTItNzQ4Y2QxNjI2OWI3___"=>4,
 "cores_per_socket-MjVlYmQ3NDctOTI5OS00MjUzLTk0YTItNzQ4Y2QxNjI2OWI3___"=>4,
 "memory_mb-MjVlYmQ3NDctOTI5OS00MjUzLTk0YTItNzQ4Y2QxNjI2OWI3___"=>2048,
 "disk_mb-MjVlYmQ3NDctOTI5OS00MjUzLTk0YTItNzQ4Y2QxNjI2OWI3___LS0tCjpkaXNrX2lkOiAnMjAwMCcK"=>4096,
 "nic_network-MjVlYmQ3NDctOTI5OS00MjUzLTk0YTItNzQ4Y2QxNjI2OWI3___LS0tCjpuaWNfaWR4OiAnMCcK"=>"RedHat Private network 43",
 "nic_mode-MjVlYmQ3NDctOTI5OS00MjUzLTk0YTItNzQ4Y2QxNjI2OWI3___LS0tCjpuaWNfaWR4OiAnMCcK"=>"DHCP",
 "nic_ip_address-MjVlYmQ3NDctOTI5OS00MjUzLTk0YTItNzQ4Y2QxNjI2OWI3___LS0tCjpuaWNfaWR4OiAnMCcK"=>nil,
 "nic_network-MjVlYmQ3NDctOTI5OS00MjUzLTk0YTItNzQ4Y2QxNjI2OWI3___LS0tCjpuaWNfaWR4OiAnMScK"=>"Default Network",
 "nic_mode-MjVlYmQ3NDctOTI5OS00MjUzLTk0YTItNzQ4Y2QxNjI2OWI3___LS0tCjpuaWNfaWR4OiAnMScK"=>"DHCP",
 "nic_ip_address-MjVlYmQ3NDctOTI5OS00MjUzLTk0YTItNzQ4Y2QxNjI2OWI3___LS0tCjpuaWNfaWR4OiAnMScK"=>nil
}

@@ -7,10 +7,11 @@ def parameter_groups
)]

# Parse template's OVF file
ovf_doc = MiqXml.load(content)
ovf_doc = MiqXml.load(content)&.root
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want this to be backported recommend staying away from &. since it is easy to miss in a cherry-pick and will break on older versions

@miha-plesko
Copy link
Contributor Author

@agrare I've just pushed some changes to this PR (namely, I've made orchestration_template_spec.rb green again). Apologies because I did it while you're reviewing it.

I'm looking forward to your comments. I do realize this PR is kindofa big, but most of them is copy-paste text parameter to be rendered in the HTML. Anyway, just let me knoiw if you want me to split it in multiple smaller PRs.

@miha-plesko miha-plesko force-pushed the vapp-provision branch 2 times, most recently from 1b77bf7 to b268f08 Compare February 27, 2018 11:01
@miha-plesko miha-plesko changed the title [WIP] Allow vApp customization prior provisioning Allow vApp customization prior provisioning Feb 27, 2018
@miha-plesko
Copy link
Contributor Author

Thanks for review @agrare, I've applied all your comments. With latest patch we now extract OVF XML parsing into its own ruby class that is being used twice:

  1. when creating service dialog out of template
  2. when processing submitted service dialog to perform reverse lookup for some template information

Looking forward to be hearing your opinion.

@miq-bot add_label gaprindashvili/yes

@@ -3,77 +3,122 @@ class Vmware::CloudManager::OrchestrationServiceOptionConverter < ::ServiceOrche
include Vmdb::Logging

def stack_create_options
@template = ManageIQ::Providers::Vmware::CloudManager::OvfTemplate.new(self.class.get_template(@dialog_options).content)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer this just be a local variable since the only users look like they are directly called from this method, you can just pass it as an argument and keep the instance vars down

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know I was actually struggling between the two options! Thanks for guidance, done.

]
}
if vm_opts.key?('disk_mb')
src_vm[:hardware][:disk] = vm_opts['disk_mb'].map { |disk| { :id => disk[:disk_id], :capacity_mb => disk[:value] } }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guessing you didn't one-line this because it was over 125 and rubocop complained :)

How about adding a parse_disks method so you can do:
src_vm[:hardware][:disk] = parse_disks(vm_opts['disk_mb']) if vm_opts.key?('disk_mb')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why auxilary parse_disks wasn't my first pick 🤔 Done.

@agrare
Copy link
Member

agrare commented Feb 27, 2018

@miha-plesko couple of little things but overall looks great, especially like ManageIQ::Providers::Vmware::CloudManager::OvfTemplate compared to what was here before

vApp provisioning is already possible via service dialogs, but
only a very modest customization is allowed. With this commit
we implement support for much more enhanced vApp customization
by allowing modification of following parameters:

- vApp name, tenant, availability zone, deploy?, power_on?
- vApp network customization (parent, fence mode, gateway, netmask, dns1, dns2)
- VM customization: name, hostname, CPU, MEM, disk size, NIC configuration (network, ip mode, ip address)

Signed-off-by: Miha Pleško <miha.plesko@xlab.si>
@miq-bot
Copy link
Member

miq-bot commented Feb 28, 2018

Checked commit miha-plesko@868d977 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 4 offenses detected

app/models/manageiq/providers/vmware/cloud_manager/ovf_template.rb

  • ❗ - Line 21, Col 5 - Style/SafeNavigation - Use safe navigation (&.) instead of checking if an object exists before calling the method.
  • ❗ - Line 27, Col 5 - Style/SafeNavigation - Use safe navigation (&.) instead of checking if an object exists before calling the method.
  • ❗ - Line 31, Col 5 - Style/SafeNavigation - Use safe navigation (&.) instead of checking if an object exists before calling the method.

spec/models/manageiq/providers/vmware/cloud_manager/ovf_template_spec.rb

Copy link
Contributor Author

@miha-plesko miha-plesko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for awesome comments @agrare. It's so comfortable applying such small optimizations when having unit tests in place!

@@ -3,77 +3,122 @@ class Vmware::CloudManager::OrchestrationServiceOptionConverter < ::ServiceOrche
include Vmdb::Logging

def stack_create_options
@template = ManageIQ::Providers::Vmware::CloudManager::OvfTemplate.new(self.class.get_template(@dialog_options).content)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know I was actually struggling between the two options! Thanks for guidance, done.

]
}
if vm_opts.key?('disk_mb')
src_vm[:hardware][:disk] = vm_opts['disk_mb'].map { |disk| { :id => disk[:disk_id], :capacity_mb => disk[:value] } }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why auxilary parse_disks wasn't my first pick 🤔 Done.

Copy link
Member

@agrare agrare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM
Thanks so much @miha-plesko for this awesome addition

@agrare agrare merged commit 868d977 into ManageIQ:master Feb 28, 2018
agrare added a commit that referenced this pull request Feb 28, 2018
Allow vApp customization prior provisioning
@agrare agrare added this to the Sprint 81 Ending Mar 12, 2018 milestone Feb 28, 2018
@miha-plesko
Copy link
Contributor Author

simaishi pushed a commit that referenced this pull request Mar 7, 2018
Allow vApp customization prior provisioning
(cherry picked from commit 27e83cc)

https://bugzilla.redhat.com/show_bug.cgi?id=1552842
@simaishi
Copy link
Contributor

simaishi commented Mar 7, 2018

Gaprindashvili backport details:

$ git log -1
commit ddc60a870027b4429ca6bfa60fddb2d167847fd5
Author: Adam Grare <agrare@redhat.com>
Date:   Wed Feb 28 07:16:23 2018 -0500

    Merge pull request #185 from miha-plesko/vapp-provision
    
    Allow vApp customization prior provisioning
    (cherry picked from commit 27e83cce1cc7e4b89b67ca3d0b2d8dc9dad92d23)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552842

@miha-plesko miha-plesko deleted the vapp-provision branch January 7, 2019 08:25
agrare pushed a commit to agrare/manageiq-providers-vmware that referenced this pull request Apr 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants