From 581f7399f9e8c967f015dc8eb57e4b361911202c Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Mon, 27 May 2024 10:21:26 -0400 Subject: [PATCH] common: use name for field access with reflect Accessing the fields of a structure through reflection is possible through several means, namely by index, or by name. Index-based access can be relevant, but is rather susceptible to reordering changes later in the life of the product, and given that it occurs at runtime only, we lose the possibility to statically detect those changes. Name-based access is slightly more robust, as we are avoiding issues like new fields changing the access, and the named fields generally change only in case of a refactor, which is rarer. Besides named accesses are somewhat self-documentational, and refect better on what we're trying to achieve in this code. Therefore, we change how the reflection accesses happen for generateProxmoxDisks, defaulting on named-based accesses instead of index-based ones for filling in the disks. --- builder/proxmox/common/step_start_vm.go | 13 +- builder/proxmox/common/step_start_vm_test.go | 155 +++++++++++++++++++ 2 files changed, 164 insertions(+), 4 deletions(-) diff --git a/builder/proxmox/common/step_start_vm.go b/builder/proxmox/common/step_start_vm.go index b2d26298..7c94eb0d 100644 --- a/builder/proxmox/common/step_start_vm.go +++ b/builder/proxmox/common/step_start_vm.go @@ -346,7 +346,6 @@ func generateProxmoxDisks(disks []diskConfig) *proxmox.QemuStorages { // similar format 'Disk_%d' FieldByName(fmt.Sprintf("Disk_%d", ideCount)). Set(reflect.ValueOf(&dev)) - reflect.ValueOf(&ideDisks).Elem().FieldByIndex([]int{ideCount}).Set(reflect.ValueOf(&dev)) ideCount++ case "scsi": dev := proxmox.QemuScsiStorage{ @@ -360,7 +359,9 @@ func generateProxmoxDisks(disks []diskConfig) *proxmox.QemuStorages { IOThread: disks[idx].IOThread, }, } - reflect.ValueOf(&scsiDisks).Elem().FieldByIndex([]int{scsiCount}).Set(reflect.ValueOf(&dev)) + reflect.ValueOf(&scsiDisks).Elem(). + FieldByName(fmt.Sprintf("Disk_%d", scsiCount)). + Set(reflect.ValueOf(&dev)) scsiCount++ case "sata": dev := proxmox.QemuSataStorage{ @@ -373,7 +374,9 @@ func generateProxmoxDisks(disks []diskConfig) *proxmox.QemuStorages { EmulateSSD: disks[idx].SSD, }, } - reflect.ValueOf(&sataDisks).Elem().FieldByIndex([]int{sataCount}).Set(reflect.ValueOf(&dev)) + reflect.ValueOf(&sataDisks).Elem(). + FieldByName(fmt.Sprintf("Disk_%d", sataCount)). + Set(reflect.ValueOf(&dev)) sataCount++ case "virtio": dev := proxmox.QemuVirtIOStorage{ @@ -386,7 +389,9 @@ func generateProxmoxDisks(disks []diskConfig) *proxmox.QemuStorages { IOThread: disks[idx].IOThread, }, } - reflect.ValueOf(&virtIODisks).Elem().FieldByIndex([]int{virtIOCount}).Set(reflect.ValueOf(&dev)) + reflect.ValueOf(&virtIODisks).Elem(). + FieldByName(fmt.Sprintf("Disk_%d", virtIOCount)). + Set(reflect.ValueOf(&dev)) virtIOCount++ } } diff --git a/builder/proxmox/common/step_start_vm_test.go b/builder/proxmox/common/step_start_vm_test.go index a901969d..7ca87200 100644 --- a/builder/proxmox/common/step_start_vm_test.go +++ b/builder/proxmox/common/step_start_vm_test.go @@ -603,6 +603,161 @@ func TestGenerateProxmoxDisks(t *testing.T) { }, }, }, + { + "bunch of disks, should be defined in the discovery order", + []diskConfig{ + { + Type: "ide", + StoragePool: "local-lvm", + Size: "10G", + CacheMode: "none", + DiskFormat: "qcow2", + IOThread: true, + }, + { + Type: "sata", + StoragePool: "local-lvm", + Size: "11G", + CacheMode: "none", + DiskFormat: "qcow2", + IOThread: true, + }, + { + Type: "ide", + StoragePool: "local-lvm", + Size: "12G", + CacheMode: "none", + DiskFormat: "qcow2", + IOThread: true, + }, + { + Type: "sata", + StoragePool: "local-lvm", + Size: "13G", + CacheMode: "none", + DiskFormat: "qcow2", + IOThread: true, + }, + { + Type: "scsi", + StoragePool: "local-lvm", + Size: "14G", + CacheMode: "none", + DiskFormat: "qcow2", + IOThread: true, + }, + { + Type: "virtio", + StoragePool: "local-lvm", + Size: "15G", + CacheMode: "none", + DiskFormat: "qcow2", + IOThread: true, + }, + { + Type: "scsi", + StoragePool: "local-lvm", + Size: "16G", + CacheMode: "none", + DiskFormat: "qcow2", + IOThread: true, + }, + { + Type: "virtio", + StoragePool: "local-lvm", + Size: "17G", + CacheMode: "none", + DiskFormat: "qcow2", + IOThread: true, + }, + }, + &proxmox.QemuStorages{ + Ide: &proxmox.QemuIdeDisks{ + Disk_0: &proxmox.QemuIdeStorage{ + Disk: &proxmox.QemuIdeDisk{ + SizeInKibibytes: 10485760, + Storage: "local-lvm", + Cache: proxmox.QemuDiskCache("none"), + Format: proxmox.QemuDiskFormat("qcow2"), + Discard: false, + }, + }, + Disk_1: &proxmox.QemuIdeStorage{ + Disk: &proxmox.QemuIdeDisk{ + SizeInKibibytes: 12582912, + Storage: "local-lvm", + Cache: proxmox.QemuDiskCache("none"), + Format: proxmox.QemuDiskFormat("qcow2"), + Discard: false, + }, + }, + }, + Sata: &proxmox.QemuSataDisks{ + Disk_0: &proxmox.QemuSataStorage{ + Disk: &proxmox.QemuSataDisk{ + SizeInKibibytes: 11534336, + Storage: "local-lvm", + Cache: proxmox.QemuDiskCache("none"), + Format: proxmox.QemuDiskFormat("qcow2"), + Discard: false, + }, + }, + Disk_1: &proxmox.QemuSataStorage{ + Disk: &proxmox.QemuSataDisk{ + SizeInKibibytes: 13631488, + Storage: "local-lvm", + Cache: proxmox.QemuDiskCache("none"), + Format: proxmox.QemuDiskFormat("qcow2"), + Discard: false, + }, + }, + }, + Scsi: &proxmox.QemuScsiDisks{ + Disk_0: &proxmox.QemuScsiStorage{ + Disk: &proxmox.QemuScsiDisk{ + SizeInKibibytes: 14680064, + Storage: "local-lvm", + Cache: proxmox.QemuDiskCache("none"), + Format: proxmox.QemuDiskFormat("qcow2"), + Discard: false, + IOThread: true, + }, + }, + Disk_1: &proxmox.QemuScsiStorage{ + Disk: &proxmox.QemuScsiDisk{ + SizeInKibibytes: 16777216, + Storage: "local-lvm", + Cache: proxmox.QemuDiskCache("none"), + Format: proxmox.QemuDiskFormat("qcow2"), + Discard: false, + IOThread: true, + }, + }, + }, + VirtIO: &proxmox.QemuVirtIODisks{ + Disk_0: &proxmox.QemuVirtIOStorage{ + Disk: &proxmox.QemuVirtIODisk{ + SizeInKibibytes: 15728640, + Storage: "local-lvm", + Cache: proxmox.QemuDiskCache("none"), + Format: proxmox.QemuDiskFormat("qcow2"), + Discard: false, + IOThread: true, + }, + }, + Disk_1: &proxmox.QemuVirtIOStorage{ + Disk: &proxmox.QemuVirtIODisk{ + SizeInKibibytes: 17825792, + Storage: "local-lvm", + Cache: proxmox.QemuDiskCache("none"), + Format: proxmox.QemuDiskFormat("qcow2"), + Discard: false, + IOThread: true, + }, + }, + }, + }, + }, } for _, tt := range tests {