-
Notifications
You must be signed in to change notification settings - Fork 454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple of minor comments - otherwise LGTM 👍
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vales
-> values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
worth making this message
to be self-explanatory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PreConfig
does not return error, so the only possible thing we can do is panic here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.