Skip to content

Commit

Permalink
common: use name for field access with reflect
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@lbajolet-hashicorp
lbajolet-hashicorp committed May 28, 2024
1 parent 34a6751 commit 581f739
Show file tree
Hide file tree
Showing 2 changed files with 164 additions and 4 deletions.
13 changes: 9 additions & 4 deletions builder/proxmox/common/step_start_vm.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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{
Expand All @@ -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++
}
}
Expand Down
155 changes: 155 additions & 0 deletions builder/proxmox/common/step_start_vm_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down

0 comments on commit 581f739

Please sign in to comment.