vatesfr / terraform-provider-xenorchestra

Xen Orchestra provider for Terraform
MIT License
151 stars 32 forks source link

adds host data source #120

Closed gohumble closed 3 years ago

gohumble commented 3 years ago

This PR adds a host data source. refers to #109.

Get host by name:

data "xenorchestra_host" "host1" {
  name_label = "xcp-host1-k8s.domain.eu"
}

output "xenorchestra_host" {
  value = data.xenorchestra_host.host1
}

Note: the source branch should be add-host-data-source.

gohumble commented 3 years ago

This is my first draft. I think CI/CD tests will probably fail. Should add XOA_HOST env variable.

I did also implement searchHostsByPoolId . This could potentially search for all hosts in a pool. I excluded this function from this data source. There are two possibilities to include this search again:

  1. Add a dedicated datasource called hosts_datasource (instead of host_datasource) and accept the pool id as parameter and return a map of all hosts.
  2. add a pool parameter to this datasource (host_datasource), and search for hosts if this parameter is set.

I think the first option is cleaner and could be separated in a new PR. @ddelnano what do you think about this ?

ddelnano commented 3 years ago

Everything looks good aside from my comment about trying to remove the need for the XOA_HOST environment variable. As I mentioned before I think we can reuse accTestPool.Master in your FindHostForTests.

gohumble commented 3 years ago

Everything looks good aside from my comment about trying to remove the need for the XOA_HOST environment variable. As I mentioned before I think we can reuse accTestPool.Master in your FindHostForTests.

I think this is not possible, because we are fetching hosts by name_label and accTestPool.Master is holding the masters UUID. Running this test with accTestPool.Master will always fail. (correct me, if I am wrong about this)

Therefore, we should consider one of the following possibilities:

  1. Extend the pool_data_source to provide a master_label and use that label on host_data_source (ugly, because xo-cli xo.getAllObjects filter=json:'{ "type": "pool" }' only provides masters uuid. )
  2. Extend the host_data_source to accept a name_label OR uuid (id).
  3. Stick with XOA_HOST environment variable.
  4. Implement the hosts_data_source first and use one of the resulting host label for host_data_source tests.

I think the last suggestion is solid and should help us to get rid of user defined input. hosts_data_source_test should fail if there are no hosts available, which also implies an error for host_data_source_test

Sorry for replying a bit late. Hope you understand my cryptic answer :D

ddelnano commented 3 years ago

I think this is not possible, because we are fetching hosts by name_label and accTestPool.Master is holding the masters UUID. Running this test with accTestPool.Master will always fail. (correct me, if I am wrong about this)

You are right accTestPool.Master is a UUID and we need a name label. I was picturing that the FindHostForTests function would do the work to fetch the entire Host struct from the UUID. This would then give us access to the name label in the test in the same way that you are currently using it.

ddelnano commented 3 years ago

@gohumble I took your branch and added a commit to it that does what I'm trying to describe. Feel free to cherry-pick that and then I think this is ready to merge.

gohumble commented 3 years ago

ahh, now I got u. Using FindFromGetAllObjectsto get the host by uuid for testing is smart. I'm pretty new to XEN/XOA and Terraform. There is a lot to learn. cerry-picked your commit into my fork. Looks good to me.

I will open a new PR for the xenorchestra_hosts data source as described in #109 (and in your comment above). But this week I'm on vacation, so it may take some time ✌🏽

ddelnano commented 3 years ago

I will open a new PR for the xenorchestra_hosts data source as described in #109 (and in your comment above). But this week I'm on vacation, so it may take some time ✌🏽

No rush and enjoy your vacation!

Thanks for all your hard work on this!