vatesfr / terraform-provider-xenorchestra

Xen Orchestra provider for Terraform
MIT License
150 stars 33 forks source link

Update hosts cpu type to integer #163

Closed gohumble closed 3 years ago

gohumble commented 3 years ago

Did find a little issue when testing #160 in combination with #161. The number of Host CPU and Sockets was mapped to a string instead of integer.

This is inconsistent, since vm's CPU information is returned as an integer as well. It also makes unwanted type casting necessary, when processing vm and host CPU values.

Sorry, I already noticed this before merging #160, and thought I mentioned it. Seems like my brain tricked me.

@ddelnano what do you think about these changes?

ddelnano commented 3 years ago

Yep this looks fine to me. #160 hasn't been included in a release yet so this isn't a breaking change yet.

I was thinking we could make a release once #160 and #161 are merged. That will give us the chance to have them tested together.

ddelnano commented 3 years ago

Actually even better would be to rebase this branch once #161 is merged and add an acceptance test that does the same sum'ing that you mentioned in #161's description (copied below).

locals {
  vmMemoryUsagePerHost = tomap({
  for k, host in data.xenorchestra_hosts.hosts.hosts : host.id => {
    max_memory : host.memory,
    usedByVm : tomap({for k, vm in data.xenorchestra_vms.vms.vms : vm.id =>  vm.memory_max if vm.container == host.id}),
    used: sum(tolist([for k, vm in data.xenorchestra_vms.vms.vms : vm.memory_max if vm.container == host.id]))
  }
  })
}

That way we can be sure that the acceptance tests are verifying this use case.

ddelnano commented 3 years ago

I misunderstood the usage here. This wouldn't have been caught with the example above. I don't think this needs additional tests at the moment.