Allow mac address to be optionally defined (#63)

[ddelnano]

Hi @njk464! Thanks for raising this feature and giving the implementation a shot.

Can you show your work for how you tested this? We will need to make sure the acceptance tests pass (and probably add a new one for this case). I can help with that since the test suite is hard coded for parts of my deployment at the moment.

[njk464]

Hi @ddelnano. I just tested it manually on our xenorchestra 5.46.0 instance with a vm resource for the cases when the mac_address field is defined, not defined, and defined incorrectly ("H"). In all cases it behaved as expected (assigning the defined mac address to the VIF, auto generating a mac address, and throwing an error). I could probably add something before the vm resource is created to verify the mac address if you want. Right now the error that's shown when the mac address is not defined is

Error: jsonrpc2: code -32000 message: unknown error from the peer

  on main.tf line 53, in resource "xenorchestra_vm" "bar":
  53: resource "xenorchestra_vm" "bar" 

when I use vm resource config of

resource "xenorchestra_vm" "bar" {
     memory_max = 8 * 1073741824 # 16GB
     cpus = 1
     cloud_config = xenorchestra_cloud_config.bar.template
     name_label = "Test Terraform"
     name_description = "description"
     template = data.xenorchestra_template.template.id
     network {
         mac_address = "H"
         network_id = data.xenorchestra_pif.Network_0.network
     }

     disk {
         sr_id = data.xenorchestra_sr.local_storage.id
         name_label = "Test Terraform"
         size = 20 * 1073741824
     }
 }

As for adding an acceptance test. It looks like I would add it in client/vm_test.go? But I don't even know where to start with making a test.

[ddelnano]

The issue with the test I identified above is because the property is no longer computed. Terraform thinks that on resource refresh the mac_address is an empty string (because the vm resource code doesn't include a mac_address attribute and it's optional) but the internals of the provider is treating it like a computed property (creating it behind the scenes and storing it in the state).

~I'm sure there is a pattern to deal with this but I don't know off the top of my head.~ oops I thought optional and computed were mutually exclusive but I was confused 🙃 .

[njk464]

Hey @ddelnano. I added the changes you requested, added the test cases you created, and updated the documentation. I'm not able to run the tests on my setup so please let me know if any other tests fail.

[ddelnano]

Since I need to run the tests and I need a little patch on top of what you have (the test code doesn't have a function it needs) I'm going to close this in favor of #65. I cherry picked your commits and the tests are passing.

Thanks for this contribution and sorry the tests aren't approachable yet! I plan to make them easy to run from a clean install soon

[njk464]

Thanks for helping me make a pull request! This feature is really going to help my team out with automating our deployment. Would love to do some more development on this project once the tests are more approachable.