luthermonson / go-proxmox

Go client with types and tests for the Proxmox-VE REST API
Apache License 2.0
154 stars 45 forks source link

json: cannot unmarshal string into Go struct field VirtualMachineConfig.cpulimit of type float32 #69

Closed ricardoalcantara closed 1 year ago

ricardoalcantara commented 1 year ago

I forgot to specify that it would came from string.

https://$PROXMOX_NODE_IP:8006/api2/json/nodes/$PROXMOX_NODE_NAME/qemu/$VMID/config

{
  "data": {
    "cores": 2,
    "name": "minha-nova-vps",
    "bootdisk": "scsi0",
    "net0": "virtio=5A:95:56:B3:18:1C,bridge=vmbr0",
    "vga": "serial0",
    "ipconfig0": "gw=192.168.0.1,ip=192.168.0.154/24",
    "memory": 2048,
    "smbios1": "uuid=5059a66c-23d5-461b-a3cb-004eb3124718",
    "boot": "c",
    "scsihw": "virtio-scsi-pci",
    "serial0": "socket",
    "ide2": "local-lvm:vm-505-cloudinit,media=cdrom,size=4M",
    "meta": "creation-qemu=7.2.0,ctime=1691463271",
    "cipassword": "**********",
    "agent": "enabled=1,fstrim_cloned_disks=1",
    "vmgenid": "d4443aa6-b66f-45b2-97ff-6dee78eade44",
    "ciuser": "root",
    "cpulimit": "0.4",
    "digest": "aa9ce37c4df320889d4ab5382a2947efa4c1d815",
    "scsi0": "local-lvm:vm-505-disk-0,size=60G"
  }
}

I actually didn't know about that, I learned if from here https://stackoverflow.com/a/9573928

I should have mapped something like this

CPULimit float32 `json:"cpulimit,omitempty,string"`

This time I actually tested, sorry for the trouble!

luthermonson commented 1 year ago

lol i like this little dig in the docs This extra level of encoding is sometimes used when communicating with JavaScript programs:

so i already accounted for this problem a couple times writing StringOr<type> types which will sort of standardize this problem in the library. the string option is a valid solution but i liked to write some of the error checking around empty values and things cause the proxmox api can be a bit sketchy on typing sometimes... maybe you want to add a StringOrFLoat32 and just use that?

ricardoalcantara commented 1 year ago

This time I tested the case where the field is empty too, so at least it's no breaking anymore. Since it's not working at the moment I would just set ,string in the end so I could keep move on my project. I am new to GoLang to suggest more elaborate solution, I have been working with it for 3 months :) .

If you are okay with this simple solution for now, I don't mind creating a new PR later.

[Edit] I also noticed that all other float variables are float64, only the one I changed is float32, so even to calculation I have to make some casts, so my suggestion is also change CPULimit to float64 together in this change, what do you think about that?

luthermonson commented 1 year ago

that's exactly what i was going to do, i started a StringOrFloat64 already... ill get a PR up and cut a new alpha release

luthermonson commented 1 year ago

so just an FYI... i tested the ,string idea and for the most part it was a mess and expected the value to always be a string and for some reason couldn't account for an empty string which was weird. the idea on the Or types was to be able to do both because I had found some APIs that were returning 1234 and another API returning a "1234" when ultimately the struct and I wanted to reuse my structs. this is just go minutia you probably don't care about but i promise you it's an interesting problem to solve!

ricardoalcantara commented 1 year ago

Thanks for your effort,I am eager to see that StringOrFloat64 implementation so I also could learn something new with it.

luthermonson commented 1 year ago

it's not as exciting as you might think :D ill merge and cut a new alpha soon

ricardoalcantara commented 1 year ago

Yes it is, I learned a lot with your pull request, I will remember it forever xD, thanks.