Add guest customization field to service catalog order form #215

gasper-vrhovsek · 2018-03-22T09:55:20Z

Added guest customization field to service catalog order
form, also added spec tests for the new field.

gasper-vrhovsek · 2018-03-22T10:44:20Z

Hi @miha-plesko @gberginc I think this is ready to be reviewed.

miha-plesko

Thanks @gasper-vrhovsek generally looks great, few minor comments then we're good to go. May I also ask you to test this on a vApp template with RHEL VM? I know for sure that provisioning fails there if guest customization is enabled and would like to make sure it works is case user unchecks this checkbox.

miha-plesko · 2018-03-23T11:04:12Z

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

+      vm.cores_per_socket    = int(el, vm_xpaths(:cores_per_socket), :default => vm.num_cores)
+      vm.memory_mb           = int(el, vm_xpaths(:memory_mb), :default => 1024)
+      vm.guest_customization = text(el, vm_xpaths(:guest_customization), :default => false)
+      vm.guest_customization = vm.guest_customization == 'true'


Can you introduce auxilary method flag() for this instead using text()?

miha-plesko · 2018-03-23T11:05:03Z

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

@@ -130,6 +132,8 @@ def vm_xpaths(key)
      "./vcloud:GuestCustomizationSection/vcloud:ComputerName"
    when :nics
      './/vcloud:NetworkConnection'
+    when :guest_customization
+      "./vcloud:GuestCustomizationSection/vcloud:Enabled"


Please use single quotes '

miha-plesko · 2018-03-23T11:10:29Z

...odels/manageiq/providers/vmware/cloud_manager/orchestration_service_option_converter_spec.rb

+      'dialog_param_memory_mb-0'           => 8192,
+      'dialog_param_admin_password-0'      => 'admin-password',
+      'dialog_param_admin_reset-0'         => 't',
+      'dialog_param_guest_customization-0' => 't',


So you've introduced this here in the input (which is BTW a simultaion what we get throught POST request when user orders provisioning). Can you please set it to 'f' in at least one case and check later in this test (L101 or L129)?

miha-plesko · 2018-03-23T11:13:57Z

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

+          :default_value => vm.guest_customization,
+          :constraints   => [
+            OrchestrationTemplate::OrchestrationParameterBoolean.new
+          ]


May I suggest we put this new checkbox above admin_password field? I think it makes more sense there, since admin password will NOT be set if we uncheck this box.

miha-plesko · 2018-03-23T11:14:33Z

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

@@ -112,6 +112,15 @@ def vm_param_groups(template)
          :constraints   => [
            OrchestrationTemplate::OrchestrationParameterBoolean.new
          ]
+        ), OrchestrationTemplate::OrchestrationParameter.new(
+          :name          => param_name('guest_customization', [vm_idx]),
+          :label         => 'Guest customization',


Label should be "Enable Guest Customization"

miha-plesko · 2018-03-23T11:16:56Z

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

+        ), OrchestrationTemplate::OrchestrationParameter.new(
+          :name          => param_name('guest_customization', [vm_idx]),
+          :label         => 'Guest customization',
+          :description   => 'Allow guest customization',


Description should be something more meaningful, please see how it's described on the vCD dashboard.

miha-plesko

Thanks @gasper-vrhovsek few more comments then I thinkg we're green to go.

miha-plesko · 2018-03-28T06:15:39Z

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

+      vm.guest_customization = bool(el, vm_xpaths(:guest_customization))
+
+      # vm.guest_customization = text(el, vm_xpaths(:guest_customization), :default => false)
+      # vm.guest_customization = vm.guest_customization == 'true'


miha-plesko · 2018-03-28T06:18:03Z

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

@@ -98,6 +102,10 @@ def parse_vapp_networks(ovf)
    end
  end

+  def bool(el, xpath, default: false)
+    text(el, xpath) == 'true' ? true : default
+  end


Does this really work as intended? If XML contains

<vcloud:Enabled>false</vcloud:Enabled>

and I call

bool(..., default: true)

then I'll get true which is wrong. The default parameter should only be returned when XML contains no entry for the xpath. So I'd rather go with:

def bool(el, xpath, default: false) (match = el.elements[xpath]) ? match.text.downcase == 'true' : default end

miha-plesko · 2018-03-28T06:29:40Z

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

+          :name          => param_name('guest_customization', [vm_idx]),
+          :label         => 'Guest customization',
+          :description   => 'The computer name and network settings configured for this VM are applied to its Guest OS
+                             when the VM is powered on. The following settings are only applied the 1st time the VM is


Can we go with this, please:

Check this to apply Hostname and NIC configuration for this VM to its Guest OS when the VM is powered on. Guest OS must support this feature otherwise provisioning will fail.

Other features that are mentioned in full text are not (yet) customizable via Service Dialog so we'd only confuse user telling her about them.

miha-plesko · 2018-03-28T06:40:55Z

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

@@ -65,6 +65,7 @@
          :memory_mb        => 2048,
          :admin_password   => nil,
          :admin_reset      => false,
+          :guest_customization => false,


I'm wondering why rubocop didn't complain about => alignment. I'd align it anyway.

agrare · 2018-04-02T15:50:10Z

@miha-plesko how does this look to you now?

miha-plesko

LGTM. Also, I've tested and it works like a charm, thanks @gasper-vrhovsek.

@agrare feel free to merge.

Added guest customization field to service catalog order form, also added spec tests for the new field.

miq-bot · 2018-04-03T08:52:17Z

Checked commit gasper-vrhovsek@e8cd22c with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. 🍪

gasper-vrhovsek · 2018-04-03T09:00:34Z

@agrare @miha-plesko last commit is just a rebase on master.

miha-plesko · 2018-04-03T11:02:33Z

@gasper-vrhovsek Travis is red because a new version of fog-vcloud-director was released which sets 5.5 as default (was 5.1 until now). Will be green once #219 gets merged

…stomization_field Add guest customization field to service catalog order form

miha-plesko · 2018-04-03T13:47:35Z

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

@miq-bot add_label enhancement,gaprindashvili/yes

…ion_field Add guest customization field to service catalog order form (cherry picked from commit 74f06e8) https://bugzilla.redhat.com/show_bug.cgi?id=1552842

simaishi · 2018-04-03T17:41:17Z

Gaprindashvili backport details:

$ git log -1
commit 215c46debd5eae9e01a3c10fe34d10b1e5791a7d
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Apr 3 09:07:06 2018 -0400

    Merge pull request #215 from gasper-vrhovsek/feature/guest_customization_field
    
    Add guest customization field to service catalog order form
    (cherry picked from commit 74f06e81252ac45b74b9ca234672250cf61e42d6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1552842

v2v: Storage mapping for mass migration

gasper-vrhovsek force-pushed the feature/guest_customization_field branch from fc42425 to c033f47 Compare March 22, 2018 10:39

miha-plesko suggested changes Mar 23, 2018

View reviewed changes

gasper-vrhovsek force-pushed the feature/guest_customization_field branch 2 times, most recently from e4207d0 to b00d6ce Compare March 26, 2018 09:19

agrare self-assigned this Mar 26, 2018

miha-plesko suggested changes Mar 28, 2018

View reviewed changes

gasper-vrhovsek force-pushed the feature/guest_customization_field branch 6 times, most recently from 696464a to 0b37900 Compare March 29, 2018 13:34

miha-plesko approved these changes Apr 3, 2018

View reviewed changes

Add guest customization field to service catalog order form

e8cd22c

Added guest customization field to service catalog order form, also added spec tests for the new field.

gasper-vrhovsek force-pushed the feature/guest_customization_field branch from 0b37900 to e8cd22c Compare April 3, 2018 08:48

miha-plesko approved these changes Apr 3, 2018

View reviewed changes

agrare closed this Apr 3, 2018

agrare reopened this Apr 3, 2018

agrare added the enhancement label Apr 3, 2018

agrare added a commit to agrare/manageiq-providers-vmware that referenced this pull request Apr 3, 2018

Merge pull request ManageIQ#215 from gasper-vrhovsek/feature/guest_cu…

74f06e8

…stomization_field Add guest customization field to service catalog order form

agrare merged commit e8cd22c into ManageIQ:master Apr 3, 2018

agrare added this to the Sprint 83 Ending Apr 9, 2018 milestone Apr 3, 2018

miq-bot added the gaprindashvili/yes label Apr 3, 2018

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Apr 3, 2018

agrare pushed a commit to agrare/manageiq-providers-vmware that referenced this pull request Apr 15, 2019

Merge pull request ManageIQ#215 from smelamud/v2v-storage-mapping

3ecc636

v2v: Storage mapping for mass migration

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add guest customization field to service catalog order form #215

Add guest customization field to service catalog order form #215

gasper-vrhovsek commented Mar 22, 2018

gasper-vrhovsek commented Mar 22, 2018

miha-plesko left a comment

miha-plesko Mar 23, 2018

miha-plesko Mar 23, 2018

miha-plesko Mar 23, 2018

miha-plesko Mar 23, 2018

miha-plesko Mar 23, 2018

miha-plesko Mar 23, 2018

miha-plesko left a comment

miha-plesko Mar 28, 2018

miha-plesko Mar 28, 2018

miha-plesko Mar 28, 2018

miha-plesko Mar 28, 2018

agrare commented Apr 2, 2018

miha-plesko left a comment

miq-bot commented Apr 3, 2018

gasper-vrhovsek commented Apr 3, 2018

miha-plesko commented Apr 3, 2018 •

edited

Loading

miha-plesko commented Apr 3, 2018

simaishi commented Apr 3, 2018

Add guest customization field to service catalog order form #215

Add guest customization field to service catalog order form #215

Conversation

gasper-vrhovsek commented Mar 22, 2018

gasper-vrhovsek commented Mar 22, 2018

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Apr 2, 2018

miha-plesko left a comment

Choose a reason for hiding this comment

miq-bot commented Apr 3, 2018

gasper-vrhovsek commented Apr 3, 2018

miha-plesko commented Apr 3, 2018 • edited Loading

miha-plesko commented Apr 3, 2018

simaishi commented Apr 3, 2018

miha-plesko commented Apr 3, 2018 •

edited

Loading