search

vatesfr / terraform-provider-xenorchestra

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

hosts data source example incomplete #185

Closed StoneMonarch closed 2 years ago

StoneMonarch commented 2 years ago

The example for affinity_host on line 24 of docs/data-sources/hosts.md is incomplete. The example tries to index the named attributes of the data source rather than applying the index to the attribute that contains the list.

In the example:

affinity_host = data.xenorchestra_hosts.hosts[count.index].id

Error:
count.index is 0
data.xenorchestra_hosts.hosts is object with 7 attributes

The given key does not identify an element in this collection value. An object only supports looking up attributes by name, not by numeric index.

Correct and working:

affinity_host = data.xenorchestra_hosts.hosts.hosts[count.index].id

My first thought was to update the documentation to show the working way but it feels a bit 'messy', having ...hosts.hosts.hosts... all right next to each other. My second thought is instead to just return a list of hosts with the master at index 0. So that way if master is needed for some reason that I cant think of, its in a known spot, and so it can have less of the same words next to each other.

You could also just use a different variable name like servers to get ...hosts.servers.hosts... but I think shortening the would still be better idea as master is still in hosts so it is returning duplicate data as well.

ddelnano commented 2 years ago

The Xen orchestra 'api object' is called a host (see screenshot below and this reference). I intentionally keep the terminology consistent in order to avoid confusion.

Screen Shot 2022-01-15 at 12 47 14 PM

My first thought was to update the documentation to show the working way but it feels a bit 'messy', having ...hosts.hosts.hosts... all right next to each other

The duplication of "hosts" is partially due to the terraform data source name. That is trivial to change and doesn't require a breaking change (data.xenorchestra_hosts.pool1.hosts rather than data.xenorchestra_hosts.hosts.hosts). The two other usages of "hosts" reflects the Xen orchestra terminology. While I understand the duplication is not ideal, I don't think deduplicating this is worth a breaking change (albeit minor).

My second thought is instead to just return a list of hosts with the master at index 0. So that way if master is needed for some reason that I cant think of, its in a known spot, and so it can have less of the same words next to each other.

The master instance is already accessible through its own attribute and by using a different data source name, there isn't any duplication (data.xenorchestra_hosts.pool1.master). Does that not serve your use case?

Changing the ordering of the hosts attribute is a larger breaking change than renaming a field. Master instances are capable of running VMs so in my mind it is appropriate to return it in the hosts field subject to the ordering and other filtering specified by the data source.

I greatly appreciate you starting this discussion, but I think the best course of action is to update the documentation to work, and rename the data source to something other than "hosts" given my reasoning above.

StoneMonarch commented 2 years ago

Yeah I did not think about the breaking change part. Documentation is probably best. PR #186