fix: Create host fails on 2.5.2 #240

tristanrobert · 2022-12-06T14:53:14Z

Fixes #207

Replace #239

Fixes #207

ekohl

I'd consider making a separate PR for the translations. There is a lot going on in this PR and that generally makes it harder to review & merge.

Personally I'd start with a PR that removes all translations from logger statements, then fix this and then a PR that adds translations.

ekohl · 2022-12-06T15:00:22Z

app/helpers/proxmox_vm_volumes_helper.rb

@@ -52,12 +49,14 @@ def parsed_typed_volumes(args, type, parsed_vm)
  end

  def parse_hard_disk_volume(args)
+    logger.debug(format(_('parse_hard_disk_volume(): args=%<args>s'), args: args))


As in the other PR: I wouldn't translate this

I suggest I remove logger statements from translation in a PR focused on i18n.

Agreed: I suggested the same in #240 (review).

ekohl · 2022-12-06T15:00:52Z

app/helpers/proxmox_vm_volumes_helper.rb

+    add_disk_options(disk, args) unless args.key?('options')
+    disk[:options] = args['options'] if args.key?('options')


I'd reduce it to a simple if/else.

I will see if theforeman-rubocop rules accept this style

ekohl · 2022-12-06T15:02:13Z

app/models/foreman_fog_proxmox/proxmox_volumes.rb

-      logger.info(format(_('vm %<vmid>s extend volume %<volume_id>s to %<extension>s'), vmid: vm.identity,
-volume_id: id, extension: extension))
+      extension = '+' + diff_size.to_s + 'G'
+      logger.info(format(_('vm %<vmid>s extend volume %<volume_id>s to %<extension>s'),


I realize it's not part of this PR, logger statements shouldn't be translated.

I suggest I remove logger statements from translation in an another PR focused on i18n.

ekohl · 2022-12-06T15:05:21Z

app/models/foreman_fog_proxmox/proxmox_volumes.rb

+      exists = false
+      return exists unless disk
+
+      exists = !volume_attributes['volid'].empty? if disk.hard_disk? || disk.cloud_init?
+      exists = !volume_attributes['cdrom'].empty? if disk.cdrom?
+      exists


I'd be much more conservative on the guard clauses. IMHO they're a poor practice that should be avoided for non-trivial things. Instead I'd write this as

Suggested change

exists = false

return exists unless disk

exists = !volume_attributes['volid'].empty? if disk.hard_disk? || disk.cloud_init?

exists = !volume_attributes['cdrom'].empty? if disk.cdrom?

exists

if !disk

false

elsif disk.hard_disk? || disk.cloud_init?

volume_attributes['volid'].presence

elsif disk.cdrom?

volume_attributes['cdrom'].presence

else

false

end

I will see if theforeman-rubocop rules accept this style

ekohl · 2022-12-06T15:07:20Z

app/models/foreman_fog_proxmox/proxmox_volumes.rb

@@ -36,7 +36,7 @@ def delete_volume(vm, id, volume_attributes)
    def volume_options(vm, id, volume_attributes)
      options = {}
      options.store(:mp, volume_attributes['mp']) if vm.container? && id != 'rootfs'
-      options.store(:cache, volume_attributes['cache']) unless vm.container?
+      options.store(:cache, volume_attributes['cache']) unless vm.container? || volume_attributes['cache'].empty?


I'd be worried about volume_attributes['cache'] being nil. In Rails you have .presence which is a safer operator:

Suggested change

options.store(:cache, volume_attributes['cache']) unless vm.container? || volume_attributes['cache'].empty?

options.store(:cache, volume_attributes['cache']) unless vm.container? || !volume_attributes['cache'].presence

And then personally I'd find this easier to follow the logic:

Suggested change

options.store(:cache, volume_attributes['cache']) unless vm.container? || volume_attributes['cache'].empty?

options.store(:cache, volume_attributes['cache']) if volume_attributes['cache'].presence && !vm.container?

ekohl · 2022-12-06T15:09:24Z

app/models/foreman_fog_proxmox/proxmox_volumes.rb

-      extension = '+' + (diff_size / GIGA).to_s + 'G'
-      logger.info(format(_('vm %<vmid>s extend volume %<volume_id>s to %<extension>s'), vmid: vm.identity,
-volume_id: id, extension: extension))
+      extension = '+' + diff_size.to_s + 'G'


Also partly unrelated, but usually this is done using string interpolation:

Suggested change

extension = '+' + diff_size.to_s + 'G'

extension = "+#{diff_size}G"

ekohl · 2022-12-06T15:11:53Z

app/models/foreman_fog_proxmox/proxmox_vm_queries.rb

-        end
+        next if vm.nil?
+        logger.debug("found vm #{vmid} on node #{node.node}")
+        break


You can write this method using find:

def find_vm_by_uuid(uuid) vmid = extract_vmid(uuid) nodes.find do |node| find_vm_in_servers_by_vmid(node.servers, vmid) || find_vm_in_servers_by_vmid(node.containers, vmid) end end

I just realized after looking: my suggestion returns the node, not the found VM so I think you can ignore my suggestion.

fix: Create host fails on 2.5.2

72fe0bf

Fixes #207

tristanrobert added the bug Something isn't working label Dec 6, 2022

tristanrobert added this to the 0.14.1 milestone Dec 6, 2022

tristanrobert self-assigned this Dec 6, 2022

tristanrobert added this to To do in ForemanFogProxmox plugin via automation Dec 6, 2022

tristanrobert mentioned this pull request Dec 6, 2022

fix: Create host fails on 2.5.2 #239

Closed

tristanrobert requested review from ekohl and removed request for ekohl December 6, 2022 15:06

tristanrobert closed this Dec 6, 2022

ForemanFogProxmox plugin automation moved this from To do to Done Dec 6, 2022

ekohl reviewed Dec 6, 2022

View reviewed changes

tristanrobert mentioned this pull request Dec 6, 2022

fix: Create host fails on 2.5.2 #241

Closed

ekohl deleted the clean_memory_size branch December 7, 2022 15:33

tristanrobert mentioned this pull request Dec 7, 2022

refactor(lang): remove logger statements from translation #242

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Create host fails on 2.5.2 #240

fix: Create host fails on 2.5.2 #240

tristanrobert commented Dec 6, 2022

ekohl left a comment

ekohl Dec 6, 2022

tristanrobert Dec 7, 2022

ekohl Dec 7, 2022

ekohl Dec 6, 2022

tristanrobert Dec 7, 2022

ekohl Dec 6, 2022

tristanrobert Dec 7, 2022 •

edited

Loading

ekohl Dec 6, 2022

tristanrobert Dec 7, 2022

ekohl Dec 6, 2022

ekohl Dec 6, 2022

ekohl Dec 6, 2022

ekohl Dec 7, 2022

		add_disk_options(disk, args) unless args.key?('options')
		disk[:options] = args['options'] if args.key?('options')

-      exists = false
-      return exists unless disk
-      exists = !volume_attributes['volid'].empty? if disk.hard_disk? || disk.cloud_init?
-      exists = !volume_attributes['cdrom'].empty? if disk.cdrom?
-      exists
+      if !disk
+        false
+      elsif disk.hard_disk? || disk.cloud_init?
+        volume_attributes['volid'].presence
+      elsif disk.cdrom?
+        volume_attributes['cdrom'].presence
+      else
+        false
+      end

	options.store(:cache, volume_attributes['cache']) unless vm.container? \|\| volume_attributes['cache'].empty?
	options.store(:cache, volume_attributes['cache']) unless vm.container? \|\| !volume_attributes['cache'].presence

	extension = '+' + diff_size.to_s + 'G'
	extension = "+#{diff_size}G"

fix: Create host fails on 2.5.2 #240

fix: Create host fails on 2.5.2 #240

Conversation

tristanrobert commented Dec 6, 2022

ekohl 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

tristanrobert Dec 7, 2022 • edited Loading

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

tristanrobert Dec 7, 2022 •

edited

Loading