Fixes #27952 - Values lost on Host and profiles edit page #43

apuntamb · 2019-09-27T09:17:23Z

As first iteration round, it fixes the existing properties to be displayed upon edit action for the form.
This PR is dependent on #42 for compute profile edit page.
Meanwhile @vijay8451 , if you can review this at your end and let me know if there are still issues. :)

vijay8451 · 2019-09-30T10:36:39Z

@apuntamb as checked the edit following are still not showing. Are they expected so far as it is or values should be there ?

VM Tab => Azure Region, Resource Group, 
Interface Tab => Azure Subnet, Public IP

apuntamb · 2019-09-30T11:22:02Z

@apuntamb as checked the edit following are still not showing. Are they expected so far as it is or values should be there ?
VM Tab => Azure Region, Resource Group, 
Interface Tab => Azure Subnet, Public IP

VM Tab => Azure Region, Resource Group should be shown now. Although, I agree the Interface Tab => Azure Subnet, Public IP is not showing. I'll look into it and update the PR. But, Region and RG should work.

vijay8451 · 2019-09-30T11:31:30Z

@apuntamb Region and RG are not working ..I am only able to see VM Size

apuntamb · 2019-10-01T12:04:27Z

app/models/foreman_azure_rm/azure_rm.rb

+                               premium_os_disk: opts[:premium_os_disk],
+                              )
+        vm = AzureRMCompute.new(azure_vm: raw_vm ,sdk: sdk)
+        vm.resource_group = opts[:resource_group]


This works well in case of Compute Profile. However, it fails for normal host creation since there it is expecting [:azure_vm][:resource_group]. Playing around with this, I could understand that keeping the above hierarchy for rg would fail for host edit and keeping just [:resource_group] would fail for profile. Can we do something here where it can handle both the scenarios?
If still it errors out, how about removing the Resource Group field from profiles page?
@ShimShtein thoughts?

kgaikwad

Thank you @apuntamb. Added few inline comments.

@ShimShtein,
I think it would be good to handle resource_group inside custom class i.e AzureRMCompute.
But I would like to know your suggestions on the same.

kgaikwad · 2019-10-01T11:52:54Z

app/models/foreman_azure_rm/azure_rm.rb

+                               premium_os_disk: opts[:premium_os_disk],
+                              )
+        vm = AzureRMCompute.new(azure_vm: raw_vm ,sdk: sdk)
+        vm.resource_group = opts[:resource_group]


resource_group is required while vm creation so instead of assigning resource_group here, would it be possible to pass it as an argument while initializing?

@kgaikwad do you mean while initializing the above custom/wrapper class, something like:
AzureRMCompute.new(azure_vm: raw_vm ,sdk: sdk, resource_group: opts[:resource_group]) ?

It still fails for create_vm. In _base partial, it still expects f.object.azure_vm.resource_group for host creation (create_vm) while it expects f.object.resource_group for compute profiles (new_vm).

Why it is expecting f.object.azure_vm.resource_group for host creation in _base partial not f.object.resource_group?
By any chance, would it be possible to override resource_group's get method & handle it accordingly?

To achieve this you have to tweak behaviour inside get method for resource_group in your custom class i.e. either @azure_vm.resource_group or resource_group whichever having value.

Yeah, I had to override resource_group's getter and handle the scenarios. Updating the changes.

app/models/foreman_azure_rm/azure_rm.rb

kgaikwad · 2019-10-01T12:48:40Z

app/models/foreman_azure_rm/azure_rm_compute.rb

+      sshkey.key_data
+    end
+
+    def premium_os_disk


Same like platform, can't you set value for premium_os_disk using @azure_vm.storage_profile.os_disk.managed_disk?

Would it be possible to fetch storage_account_type value from managed_disk?

app/views/compute_resources_vms/form/azurerm/_base.html.erb

kgaikwad · 2019-10-03T11:06:01Z

@apuntamb,
From next time, could you please push new changes into separate commits?
It would be easy to review new version of suggested changes.
If you want, you can squash it at the end.

apuntamb · 2019-10-03T11:10:02Z

@apuntamb,
From next time, could you please push new changes into separate commits?
It would be easy to review new version of suggested changes.
If you want, you can squash it at the end.

Sure thing :)

apuntamb · 2019-10-03T13:16:45Z

@apuntamb Region and RG are not working ..I am only able to see VM Size

@vijay8451 Can you please try now. I have updated the PR. This should work for both Host edit page and Compute Profiles edit page. However, Network Interfaces tab values might not still work.

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

app/models/foreman_azure_rm/azure_rm_compute.rb

ShimShtein · 2019-10-06T12:44:23Z

app/models/foreman_azure_rm/azure_rm_compute.rb

@@ -22,6 +33,39 @@ def vm_size
      @azure_vm.hardware_profile.vm_size
    end

+    def platform


Can we either separate azure-specific fields like platform to a concern, or at least put a comment that this property is a convenience for us, and not part of foreman's interface?

Makes sense. I'll add comment here.

ShimShtein · 2019-10-06T13:32:03Z

app/models/foreman_azure_rm/azure_rm.rb

@@ -155,7 +180,9 @@ def vm_sizes(region)
    end

    def vm_instance_defaults
-      ActiveSupport::HashWithIndifferentAccess.new
+      super.merge(


deep_merge suits better - if there are some properties defined on interfaces in super, we still want them. Regular merge will replace the whole interfaces member.

app/views/compute_resources_vms/form/azurerm/_network.html.erb

vijay8451 · 2019-10-07T05:51:50Z

@apuntamb Region and RG are not working ..I am only able to see VM Size

@vijay8451 Can you please try now. I have updated the PR. This should work for both Host edit page and Compute Profiles edit page. However, Network Interfaces tab values might not still work.

@apuntamb I'll start once code refactor part complete :)

apuntamb · 2019-10-07T12:04:59Z

app/views/compute_resources_vms/form/azurerm/_network.html.erb

-                 { :include_blank => _('Please first select an image and an Azure region')},
+<%= selectable_f f, :network, [],#options_for_select(compute_resource.subnets("eastus")),
+                 { :include_blank => _('Please first select an image and an Azure region'),
+                   :selected => nil#f.object.network


This f.object.network is constructed after the vm is created. So, doesn't work for #new_vm method. This f comes from the new_interface method which returns nothing but the azure's (sdk's) NetworkInterface.new. How should we display the value for this on edit page or Profile?

apuntamb · 2019-10-17T15:00:41Z

As per the commit
fb07d60 , there is a structural change for AzureRM compute resource. The Compute resource will now have region as an attribute and hence, region will be available throughout, on the compute resource object under consideration.
This is a much consistent approach if compared with all other compute resources that foreman supports.
Also, now we are at the stage where the values on edit host or compute profiles page for size, resource_group or subnets are also available.
@vijay8451 I would like you to now start testing since we have the code ready.
Also, @kgaikwad will be helping us with the review here, including some duplicate code refactoring (if present).

vijay8451 · 2019-10-18T11:29:17Z

@apuntamb test results

Works:
Test Connection
Create Compute Resource with Region attribute set
Create Image with User data ENABLED
Create Image with User data DISABLED
Go to VM tab directly (no dependency of OS tab for VM tab)
Create VM with password (Finish Template)
Create VM with sshkey (Finish Template)
Create VM with password (User data Template)
Create VM with sshkey (User data Template)
Create VM with password with custom script (Finish Template)
Create VM with sshkey with custom script (Finish Template)
Create VM with password with custom script (User data Template)
Create VM with sshkey with custom script (User data Template)
Edit Host Page opens
Values on Edit host page present
Network Interfaces tab values on edit host page present
Power On works
Power Off works
Destroy host/vm present in azure portal
Destroy host/vm NOT present in azure portal
Create Compute Profile for Azure Compute Resource
Edit Compute Profile for Azure Compute Resource
Values present on Edit Compute profile page
List all VMs in Virtual Machines tab on Compute Resource page
Diassociate VM

Not works:
Compute Resourse:
In CR VM list should show region wise
Region should not be hardcoded

Create Image:
Provided password get blank after second time edit
Create image does not have formate validation for provided URN

Compute Profile:
Selected Image and password get blank after save

Not works are not a blocker for this PR but requires to be fix and can handle separately.
hence ACK from QE.

…esource

apuntamb · 2019-10-21T12:27:15Z

@vijay8451 Thanks for the thorough testing!
In the latest commit, the VMs list will be shown region wise under ComputeResource page.
Also, now AssociateVM should work smoothly too. Kindly try de-associating followed by associating the vm. Also, we can keep the region hardcoded for now, as it doesn't depend on the Test Connection operation.
For the Compute profile, I cannot reproduce the issue of image getting blank. Password shall show ******* after save but, Image value will be set on edit page.
For Image validation, I have for now put the help text for users.

vijay8451 · 2019-10-22T10:34:30Z

@vijay8451 Thanks for the thorough testing!
In the latest commit, the VMs list will be shown region wise under ComputeResource page.

Able to see now .

Also, now AssociateVM should work smoothly too. Kindly try de-associating followed by associating the vm.

Works fine when click on Associate VMs under CR , However a feature still requires to add where a specific VM can Associate too.(But that more looks a enhancement and can handle under different PR )

Also, we can keep the region hardcoded for now, as it doesn't depend on the Test Connection operation.

Okay, If i understood correctly , so we have a plan to move from hardcoded to automate way in near future ?

For the Compute profile, I cannot reproduce the issue of image getting blank. Password shall show ******* after save but, Image value will be set on edit page.

I tired to latest dev setup and found :
a. Selected Image, password, 'Custom Script Command', 'Comma seperated file URIs' get blank after save.
b. Unable to click on "Add Interface button" under 'Network interfaces'.
c. Using same compute profile Host Create fails with following trace back:

05:52:10 rails.1   | 2019-10-22T05:52:10 [D|app|1b85a0b6] Backtrace for 'Failed to create a compute CR_Test1 (Azure Resource Manager) instance elva-giblin.example.com: undefined method `start_with?' for nil:NilClass
05:52:10 rails.1   |  |  ' error (NoMethodError): undefined method `start_with?' for nil:NilClass
05:52:10 rails.1   |  | /home/vagrant/foreman_azure_rm/app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb:135:in `block in initialize_vm'
05:52:10 rails.1   |  | /home/vagrant/foreman_azure_rm/app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb:126:in `tap'
05:52:10 rails.1   |  | /home/vagrant/foreman_azure_rm/app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb:126:in `initialize_vm'

For Image validation, I have for now put the help text for users.

Works for me now .

apuntamb · 2019-10-22T14:22:25Z

@vijay8451 I have pushed a commit that fixes the associate vm and network interface problem for compute profile.
Although, I would like @ShimShtein and @kgaikwad to review the PR code-wise so, that we do not keep on retesting functionalities before and after refactoring. How does that sound?

vijay8451 · 2019-10-22T14:29:01Z

@vijay8451 I have pushed a commit that fixes the associate vm and network interface problem for compute profile.
Although, I would like @ShimShtein and @kgaikwad to review the PR code-wise so, that we do not keep on retesting functionalities before and after refactoring. How does that sound?

@apuntamb sounds good to me .

app/models/foreman_azure_rm/azure_rm_compute.rb

ShimShtein · 2019-10-23T06:52:51Z

lib/foreman_azure_rm/engine.rb

@@ -4,13 +4,14 @@ class Engine < ::Rails::Engine

    #autoloading all files inside lib dir
    config.autoload_paths += Dir["#{config.root}/lib"]
+    config.autoload_paths += Dir["#{config.root}/lib/foreman_azure_rm"]


You don't need this directive, If it is present class ::Foo could be resolved to file lib/foreman_azure_rm/foo.rb. I don't think this is what we want.

ShimShtein · 2019-10-23T07:23:41Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

-          attrs[:privip_alloc] = (attrs[:name] == 'false') ? false : true
-          pip_alloc            = case attrs[:pubip_alloc]
+          private_ip = (attrs[:private_ip] == 'false') ? false : true
+          pip_alloc            = case attrs[:public_ip]


Can we call it pub_ip? pip can be both public and private.

ShimShtein · 2019-10-23T08:22:52Z

app/models/foreman_azure_rm/azure_rm.rb

-        sdk.vnets.select { |vnet| vnet.location == stripped_region }
-      end
+    def virtual_networks
+      sdk.vnets.select { |vnet| vnet.location == region }


Since the region is constant, I think it's worth memoizing the result:

Suggested change

sdk.vnets.select { |vnet| vnet.location == region }

@virtual_networks ||= sdk.vnets.select { |vnet| vnet.location == region }

kgaikwad · 2019-10-23T07:06:38Z

app/models/foreman_azure_rm/azure_rm.rb

-    def new_vm(attr = {})
-      AzureRMCompute.new(sdk: sdk)
+    def new_vm(args = {})
+      if args.empty? || args[:image_id].nil?


Instead of adding if..else, could you please add return statement with if condition. Something like:
return AzureRMCompute.new(sdk: sdk) if args.empty? || args[:image_id].nil?

kgaikwad · 2019-10-23T07:09:42Z

app/models/foreman_azure_rm/azure_rm.rb

+            ifaces << new_interface(iface_attrs)
+          end
+        end
+        vm = AzureRMCompute.new(azure_vm: raw_vm ,sdk: sdk, resource_group: opts[:resource_group], nics: ifaces)


Here, no need of assignment. In Ruby, methods always return the evaluated result of the last line of the expression unless an explicit return comes before it.

Suggested change

vm = AzureRMCompute.new(azure_vm: raw_vm ,sdk: sdk, resource_group: opts[:resource_group], nics: ifaces)

AzureRMCompute.new(azure_vm: raw_vm ,sdk: sdk, resource_group: opts[:resource_group], nics: ifaces)

kgaikwad · 2019-10-23T08:52:28Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

@@ -35,7 +35,7 @@ def define_managed_storage_profile(vm_name, vhd_path, publisher, offer, sku, ver
        os_disk.managed_disk = managed_disk_params
        storage_profile.os_disk = os_disk

-        # Currently eliminating data disk creation since capability does not exist.
+        # ToDo disk creation for volume capability


Suggested change

# ToDo disk creation for volume capability

# TODO - disk creation for volume capability

Could you please create an upstream issue under Compute resources - Azure category for better tracking instead of having it here?

will create a tracker.

kgaikwad · 2019-10-23T09:01:15Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

+        vm_params = initialize_vm(vm_hash)
+        vm_params.network_profile = define_network_profile(vm_hash[:network_interface_card_ids])
+
+        actual_vm = sdk.create_or_update_vm(vm_hash[:resource_group], vm_hash[:name], vm_params)
        logger.debug "Virtual Machine #{vm_hash[:name]} Created Successfully."


Please move this logger statement inside azure_rm.rb after this line.

kgaikwad · 2019-10-23T09:03:55Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

        logger.debug "Virtual Machine #{vm_hash[:name]} Created Successfully."
-        response
+        actual_vm


Instead of lines(194..196), simply return -

sdk.create_or_update_vm(vm_hash[:resource_group], vm_hash[:name], vm_params)

ShimShtein · 2019-10-23T11:39:18Z

app/models/foreman_azure_rm/azure_rm.rb

+      return AzureRMCompute.new(sdk: sdk) if args.empty? || args[:image_id].nil?
+      opts = vm_instance_defaults.merge(args.to_h).deep_symbolize_keys
+      # convert rails nested_attributes into a plain hash
+      [:interfaces].each do |collection|


Do we still need this generic code, or can we simplify it to:

nested_args = opts.delete(:interfaces_attributes) opts[collection] = nested_attributes_for(:interfaces, nested_args) if nested_args

app/views/compute_resources_vms/form/azurerm/_base.html.erb

ShimShtein

ACK from me.
@vijay8451, can you please give it a run? If you're OK with it, we can merge it

vijay8451 · 2019-10-23T14:18:44Z

@apuntamb

Below are not working :

Compute Profile:

Selected Image, password, 'Custom Script Command', 'Comma seperated file URIs' get blank after save
Unable to see Network interfaces

Below comes during profile edit:

VM Power on/off/Edit/Delete action always end with below screen:

apuntamb · 2019-10-23T16:11:05Z

@vijay8451 Following is my Compute Profiles Edit page. I can see Network Interfaces tab, Password and Image both are set after save.
About Custom Script and URIs, I can't see. Need to fix this.

apuntamb · 2019-10-23T17:13:29Z

@vijay8451 Ohkk I see that network interfaces for profiles is not loading.
@ShimShtein This is caused because nics if passed as empty [], it doesn't load the network interfaces partial for compute profiles and hence, the nics were earlier initialized with
nics: [OpenStruct.new(:network => "", :public_ip => "", :private_ip => false)].

kgaikwad

@apuntamb, added few inline comments not major.

kgaikwad · 2019-10-24T07:34:14Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

                  nic_conf.private_ipallocation_method = priv_ip_alloc
-                  nic_conf.subnet = subnets(args[:azure_vm][:location]).select{ |subnet| subnet.id == attrs[:network] }.first
+                  nic_conf.subnet = subnets.select{ |subnet| subnet.id == attrs[:network] }.first
                  nic_conf.public_ipaddress = pip.present? ? pip : nil


Suggested change

nic_conf.public_ipaddress = pip.present? ? pip : nil

nic_conf.public_ipaddress = pip

pip will be nil if pub_ip_alloc not present, right? so to add condition here doesn't make any sense.

kgaikwad · 2019-10-24T07:37:21Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

                                   when 'Static'
                                     NetworkModels::IPAllocationMethod::Static
                                   when 'Dynamic'
                                     NetworkModels::IPAllocationMethod::Dynamic
                                   when 'None'
                                     nil
                                 end
-          priv_ip_alloc        = if attrs[:priv_ip_alloc]
+          priv_ip_alloc        = if private_ip


As this priv_ip_alloc assignment is not dependent on pub_ip_alloc, could you please move this block above before pub_ip_alloc assignment to make it better readable?

Currently it looks like -
private_ip value then pub_ip_alloc assignment -> priv_ip_alloc assignment -> again a condition based on pub_ip_alloc.

kgaikwad · 2019-10-24T07:49:45Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

@@ -120,7 +119,7 @@ def create_nics(args = {})
        nics
      end

-      def create_managed_virtual_machine(vm_hash)
+      def initialize_vm(vm_hash)
        custom_data = vm_hash[:custom_data]


I can see that custom_data used only once in this method, do we really need to declare this custom_data?
why not to use vm_hash[:custom_data] directly wherever required.

I don't think this should be a part of this PR. I would like to address functionality issues here, and refactor the code later on, while we are not under delivery pressure.

kgaikwad · 2019-10-24T07:58:40Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

@@ -120,7 +119,7 @@ def create_nics(args = {})
        nics
      end

-      def create_managed_virtual_machine(vm_hash)
+      def initialize_vm(vm_hash)
        custom_data = vm_hash[:custom_data]
        msg = "Creating Virtual Machine #{vm_hash[:name]} in Resource Group #{vm_hash[:resource_group]}."
        logger.debug msg


At line-128,

if vm_hash[:availability_set_id] sub_resource = MsRestAzure::SubResource.new sub_resource.id = vm_hash[:availability_set_id] vm.availability_set = sub_resource end

looks much better, right?

Again, it's cosmetics in the original code, let's leave it to after delivery.

kgaikwad · 2019-10-24T08:06:24Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

@@ -183,30 +182,34 @@ def create_managed_virtual_machine(vm_hash)
          vm.hardware_profile = ComputeModels::HardwareProfile.new.tap do |hw_profile|
            hw_profile.vm_size = vm_hash[:vm_size]
          end
-          vm.network_profile = define_network_profile(vm_hash[:network_interface_card_ids])
        end


Instead of hard-coding ssh_key path at two places, could you initialize a variable with given path.
Then use that variable at these places.

I think there is only one place with hard-coded path right now.

Line number 156 & 163.

kgaikwad · 2019-10-24T08:11:57Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

-        if args[:azure_vm][:script_command].present? || args[:azure_vm][:script_uris].present?
+      def create_vm_extension(region, args = {})
+        if args[:script_command].present? || args[:script_uris].present?
+          args[:script_uris] = args[:script_uris].to_s unless args[:script_uris].present?


Suggested change

args[:script_uris] = args[:script_uris].to_s unless args[:script_uris].present?

args[:script_uris] ||= args[:script_uris].to_s

This will fail if the value is nil (it would be nil.to_s)

In that case, current statement will also fail, right?

@ShimShtein It will not fail as nil.to_s will give us "".

kgaikwad · 2019-10-24T08:21:23Z

app/models/foreman_azure_rm/azure_rm_compute.rb

+      public_ip_address || private_ip_address
+    end
+
+    def public_ip_address
      interfaces.each do |nic|
        nic.ip_configurations.each do |configuration|
          next unless configuration.primary


Why not to return nil after this when configuration.public_ipaddress is not present.

I'm returning nil here if the public ip is not present: line 86

I know that you have returned nil in case public_ipaddress is not present.
What I meant to say was that instead of having if.. else loop why not to mention something like -

return nil if configuration.public_ipaddress.blank?

kgaikwad · 2019-10-24T08:53:47Z

app/models/foreman_azure_rm/azure_rm.rb

+    def vm_nics(vm)
+      ifaces = []
+      vm.network_profile.network_interfaces.each do |nic|
+        nic_rg = nic.id.split('/')[4]


Suggested change

nic_rg = nic.id.split('/')[4]

nic_rg = ((splits_nic_id = nic.id.split('/'))[4]

nic_name = splits_nic_id[-1]

kgaikwad · 2019-10-24T09:06:00Z

app/models/foreman_azure_rm/azure_rm.rb

-      stripped_region = region.gsub(/\s+/, '').downcase
-      vnets   = virtual_networks(stripped_region)
+    def subnets
+      vnets   = virtual_networks
      subnets = []
      vnets.each do |vnet|
        subnets.concat(sdk.subnets(vnet.resource_group, vnet.name))
      end
      subnets
    end


Currently method available_subnets internally calls to subnets then why not to it alias_method of subnets?

alias_method :available_subnets, :subnets

kgaikwad · 2019-10-24T09:15:18Z

app/models/foreman_azure_rm/azure_rm.rb

    rescue RuntimeError => e
      Foreman::Logging.exception('Unhandled Azure RM error', e)
      destroy_vm vm.id if vm
      raise e
    end

    def destroy_vm(uuid)
-      #vm.azure_vm because that's the azure object and vm is the wrapper
      vm           = find_vm_by_uuid(uuid)
      vm_name      = vm.name


Why not to use vm.name directly as used only once? Similar case for nic_ids.

ShimShtein

@kgaikwad, I would prefer doing those cosmetic changes in a different PR. I think this PR is already very big, to add more code that is not contributing to functionality.

@apuntamb, I have found how to force nic template to appear properly in compute profile.

All in all it looks good.

ShimShtein · 2019-10-30T10:55:29Z

app/models/foreman_azure_rm/azure_rm.rb

-      # causes compute profiles issue
-      # NetworkModels::NetworkInterface.new
+    def new_interface(attrs = {})
+      args = { :network => "", :public_ip => "", :private_ip => false }.merge(attrs.to_h)


Suggested change

args = { :network => "", :public_ip => "", :private_ip => false }.merge(attrs.to_h)

args = { :network => "", :public_ip => "", :private_ip => false, 'persisted?' => false }.merge(attrs.to_h)

@apuntamb, this will make fields_for generate new interface template in compute profiles.

@ShimShtein This worked like a charm.

ShimShtein · 2019-10-30T16:00:32Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

@@ -120,7 +119,7 @@ def create_nics(args = {})
        nics
      end

-      def create_managed_virtual_machine(vm_hash)
+      def initialize_vm(vm_hash)
        custom_data = vm_hash[:custom_data]


I don't think this should be a part of this PR. I would like to address functionality issues here, and refactor the code later on, while we are not under delivery pressure.

ShimShtein · 2019-10-30T16:01:44Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

@@ -120,7 +119,7 @@ def create_nics(args = {})
        nics
      end

-      def create_managed_virtual_machine(vm_hash)
+      def initialize_vm(vm_hash)
        custom_data = vm_hash[:custom_data]
        msg = "Creating Virtual Machine #{vm_hash[:name]} in Resource Group #{vm_hash[:resource_group]}."
        logger.debug msg


Again, it's cosmetics in the original code, let's leave it to after delivery.

ShimShtein · 2019-10-30T16:11:35Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

@@ -183,30 +182,34 @@ def create_managed_virtual_machine(vm_hash)
          vm.hardware_profile = ComputeModels::HardwareProfile.new.tap do |hw_profile|
            hw_profile.vm_size = vm_hash[:vm_size]
          end
-          vm.network_profile = define_network_profile(vm_hash[:network_interface_card_ids])
        end


I think there is only one place with hard-coded path right now.

ShimShtein · 2019-10-30T16:12:49Z

app/models/concerns/foreman_azure_rm/vm_extensions/managed_vm.rb

-        if args[:azure_vm][:script_command].present? || args[:azure_vm][:script_uris].present?
+      def create_vm_extension(region, args = {})
+        if args[:script_command].present? || args[:script_uris].present?
+          args[:script_uris] = args[:script_uris].to_s unless args[:script_uris].present?


This will fail if the value is nil (it would be nil.to_s)

kgaikwad · 2019-10-30T17:04:09Z

@ShimShtein,
+1 surely! I don't mind having those changes in a different pull-request.

vijay8451 · 2019-10-31T09:58:33Z

@apuntamb @ShimShtein @kgaikwad
Not further blocker seen ..Below can handle under separate PR .

In CP, 'Selected Image', 'Custom Script Command', 'Comma seperated file URIs' get blank after save

LGTM 👍 ... ACK from QE

ShimShtein · 2019-10-31T11:56:20Z

Merged. 🎉
Awesome work, @apuntamb and @vijay8451!

Thanks a lot!

apuntamb requested a review from ShimShtein September 27, 2019 09:17

apuntamb force-pushed the 27952_values_lost branch 2 times, most recently from 28acd05 to a0d6615 Compare September 27, 2019 10:23

apuntamb commented Oct 1, 2019

View reviewed changes

kgaikwad suggested changes Oct 1, 2019

View reviewed changes

apuntamb force-pushed the 27952_values_lost branch from a0d6615 to 0fbab32 Compare October 3, 2019 10:51

apuntamb force-pushed the 27952_values_lost branch from 0fbab32 to ab73dc4 Compare October 3, 2019 13:06

ShimShtein requested changes Oct 6, 2019

View reviewed changes

ShimShtein mentioned this pull request Oct 6, 2019

Update API params for AzureRM #41

Merged

apuntamb commented Oct 7, 2019

View reviewed changes

apuntamb added 2 commits October 11, 2019 05:54

Fixes #27952 - Values lost on Host and profiles edit page

caaab3b

refactored resource group

1e26f18

apuntamb mentioned this pull request Oct 11, 2019

Fixes #27621 - Click on Compute profile fails #42

Closed

handling changes from theforeman#42

f9095d2

apuntamb force-pushed the 27952_values_lost branch from 50ab426 to f9095d2 Compare October 11, 2019 06:09

fixed premium_os_disk and commented for azure properties

50f9646

apuntamb force-pushed the 27952_values_lost branch 2 times, most recently from 72e07b8 to cbd94f8 Compare October 18, 2019 10:28

Refactored Interfaces and Added Region attribute on AzureRM compute r…

7921d91

…esource

apuntamb force-pushed the 27952_values_lost branch from cbd94f8 to 7921d91 Compare October 21, 2019 12:20

fixes compute profile interfaces and associate vm

f9e94c8

apuntamb force-pushed the 27952_values_lost branch from a90da40 to f9e94c8 Compare October 22, 2019 18:26

ShimShtein requested changes Oct 23, 2019

View reviewed changes

kgaikwad suggested changes Oct 23, 2019

View reviewed changes

ShimShtein requested changes Oct 23, 2019

View reviewed changes

apuntamb force-pushed the 27952_values_lost branch from 42e6d4a to 460e88a Compare October 23, 2019 12:32

Refactoring code

68d56f0

apuntamb force-pushed the 27952_values_lost branch from 460e88a to 68d56f0 Compare October 23, 2019 12:56

ShimShtein approved these changes Oct 23, 2019

View reviewed changes

kgaikwad suggested changes Oct 24, 2019

View reviewed changes

ShimShtein requested changes Oct 30, 2019

View reviewed changes

changes for nics and minor refactoring

da2a585

apuntamb force-pushed the 27952_values_lost branch from 791a389 to da2a585 Compare October 31, 2019 07:00

ShimShtein approved these changes Oct 31, 2019

View reviewed changes

ShimShtein merged commit d40391d into theforeman:master Oct 31, 2019

	sdk.vnets.select { \|vnet\| vnet.location == region }
	@virtual_networks \|\|= sdk.vnets.select { \|vnet\| vnet.location == region }

	vm = AzureRMCompute.new(azure_vm: raw_vm ,sdk: sdk, resource_group: opts[:resource_group], nics: ifaces)
	AzureRMCompute.new(azure_vm: raw_vm ,sdk: sdk, resource_group: opts[:resource_group], nics: ifaces)

	# ToDo disk creation for volume capability
	# TODO - disk creation for volume capability

	nic_conf.public_ipaddress = pip.present? ? pip : nil
	nic_conf.public_ipaddress = pip

	args[:script_uris] = args[:script_uris].to_s unless args[:script_uris].present?
	args[:script_uris] \|\|= args[:script_uris].to_s

	nic_rg = nic.id.split('/')[4]
	nic_rg = ((splits_nic_id = nic.id.split('/'))[4]
	nic_name = splits_nic_id[-1]

	args = { :network => "", :public_ip => "", :private_ip => false }.merge(attrs.to_h)
	args = { :network => "", :public_ip => "", :private_ip => false, 'persisted?' => false }.merge(attrs.to_h)

Fixes #27952 - Values lost on Host and profiles edit page #43

Fixes #27952 - Values lost on Host and profiles edit page #43

Conversation

apuntamb commented Sep 27, 2019 • edited

vijay8451 commented Sep 30, 2019

apuntamb commented Sep 30, 2019

vijay8451 commented Sep 30, 2019

Choose a reason for hiding this comment

kgaikwad 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

kgaikwad Oct 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaikwad Oct 1, 2019 • edited

Choose a reason for hiding this comment

kgaikwad commented Oct 3, 2019

apuntamb commented Oct 3, 2019

apuntamb commented Oct 3, 2019

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

vijay8451 commented Oct 7, 2019

Choose a reason for hiding this comment

apuntamb commented Oct 17, 2019

vijay8451 commented Oct 18, 2019 • edited

apuntamb commented Oct 21, 2019

vijay8451 commented Oct 22, 2019

apuntamb commented Oct 22, 2019 • edited

vijay8451 commented Oct 22, 2019

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

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

kgaikwad Oct 23, 2019 • edited

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

ShimShtein left a comment

Choose a reason for hiding this comment

vijay8451 commented Oct 23, 2019 • edited

apuntamb commented Oct 23, 2019 • edited

apuntamb commented Oct 23, 2019

kgaikwad 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaikwad Oct 31, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apuntamb commented Sep 27, 2019 •

edited

kgaikwad Oct 3, 2019 •

edited

kgaikwad Oct 1, 2019 •

edited

vijay8451 commented Oct 18, 2019 •

edited

apuntamb commented Oct 22, 2019 •

edited

kgaikwad Oct 23, 2019 •

edited

vijay8451 commented Oct 23, 2019 •

edited

apuntamb commented Oct 23, 2019 •

edited

kgaikwad Oct 31, 2019 •

edited