search

vatesfr / terraform-provider-xenorchestra

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

Set the ha and autopoweron options while creating vms #213

Closed 4censord closed 11 months ago

4censord commented 1 year ago

This fixes #209

ddelnano commented 1 year ago

@4censord apologies for the late reply! I'm planning to be more active in the project and appreciate your contribution.

I'm currently working on getting a CI job setup to make reviewing these easier (running the test suite is very painful and time consuming at the moment). So I'll be giving this a review soon (in the next week).

4censord commented 1 year ago

It seem to not have been caught by the tests that this was broken, should we add a new test to check that it is working now?

ddelnano commented 1 year ago

@4censord validating this functionality with a test would be great. We already have the TestAccXenorchestraVm_updatesWithoutReboot test, which would be a good fit for verifying this functionality.

Can you update this PR to include that?

4censord commented 1 year ago

I am unsure whether this is actually the right test to use for this. The issue did only surface on the initial creation set, but this tests if modifications work.

We could change the test so that it creates the vm with ha and autopoweron and later removes those. But i'm not sure if that's the right course.

4censord commented 1 year ago

Do we have a test that creates a vm with all parameters set to non default values? That would be the ideal place for this imo.

ddelnano commented 1 year ago

I am unsure whether this is actually the right test to use for this. The issue did only surface on the initial creation set, but this tests if modifications work.

Sorry, I misunderstood where the discrepancy was.

We don't have a test that sets all parameters to non default values. That works or we can create a more specific test. I think having a more makes it easier to determine what could be wrong during a failure is if a test fails but either approach is fine with me.

ryanlelek commented 1 year ago

This commit is small and looks good to me. Is there any specific testing you'd like me to do to be more comfortable merging this?

4censord commented 1 year ago

What is missing now is a test to verify that this works.

Basicaly creating a vm with both parameters set, and check that they are set after the fist apply.

If you'd like to add this, please feel free.

I currently have neither the time, nor motivation, nor a cluster for running the tests on.

sboyd-m commented 11 months ago

I was able to test this, and it does indeed set autopoweron and HA. Would really advocate this being merged, as it is an annoyance for us as well.

ddelnano commented 11 months ago

I'll be able to pick up adding a test for this in the upcoming days. One example for why I believe adding a test is a crucial and necessary step, is that the XO api client layer needs to be significantly reworked due to the breaking change made by the XO APi (details in #238 and linked issues). Without this test, I think it's likely that a small fix like this will be broken during that large refactor.

I understand this functionality is important and that this has been an annoyance, but this fix should be coming soon.

ddelnano commented 11 months ago

The test changes ended up being easy to fix (#252). Reordering which changes are applied first ensures that the creation with a non default value is covered.

I'll have to look into creating a new release soon. Hopefully that can happen within the next week or so.