r/virtual_machine: Remove OOB deleted virtual machines from state #321
Conversation
This was incorrectly changed from v0.4.2. While failing on read errors is normal, it's a TF convention to check to see if the read error was due to a resource being deleted outside of the TF workflow, and if it has, mark that resource as gone by blanking out its ID. This update restores that behavior to the VM resource. Other resources in the provider will need to have this functionality added, although most of those items are core infrastructure resources and are not as ephemeral as a virtual machine is, so they are probably lower priority to update. They will be done on an as needed basis. Fixes #299.
|
Test in question: |
vsphere/helper_test.go
Outdated
| // copyState returns a TestCheckFunc that returns a deep copy of the state. | ||
| // Unlike copyStatePtr, this state has de-coupled from the in-flight state, so | ||
| // it will not be modified on subsequent steps and hence will possibly drift. | ||
| // It can be used to access vales of the state at a certain step. |
| // UUIDNotFoundError is an error type that is returned when a | ||
| // virtual machine could not be found by UUID. | ||
| type UUIDNotFoundError struct { | ||
| s string |
There was a problem hiding this comment.
worth making this message to be self-explanatory?
There was a problem hiding this comment.
This is an internal field on a very small object - and this generally matches the pattern you see in something like the errors package, so I think it's fine :)
| PreConfig: func() { | ||
| if err := testDeleteVM(state, "vm"); err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
maybe it's just and Azure provider thing - but we'd generally make the deletion a Check function and assert it's been deleted, rather than crashing?
There was a problem hiding this comment.
PreConfig does not return error, so the only possible thing we can do is panic here.
There was a problem hiding this comment.
Update: The intention here is to make sure this gets handled completely outside of the plan/apply testing cycle. PreConifg happens very early on, which is why I use it - Check on the other hand happens pretty much in the middle after the apply, but before the post-apply refresh to check for an empty plan. Having these OOB steps in PreConfig kind of gives them a more real-world workflow (ie: someone usually makes manual modifications between a TF run, versus in the middle of one).
What would be nicer is if the signature of PreConfig was altered to send an error back, but that's kind of a breaking change.
ghost
locked and limited conversation to collaborators
This was incorrectly changed from v0.4.2. While failing on read errors
is normal, it's a TF convention to check to see if the read error was
due to a resource being deleted outside of the TF workflow, and if it
has, mark that resource as gone by blanking out its ID.
This update restores that behavior to the VM resource. Other resources
in the provider will need to have this functionality added, although
most of those items are core infrastructure resources and are not as
ephemeral as a virtual machine is, so they are probably lower priority
to update. They will be done on an as needed basis.
Fixes #299.