r/virtual_machine: Fix broken acceptance tests and CD drives #427
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This does a few things: * Looks like some acceptance test were broken - first one was due to something changing in RancherOS that does not allow for the proper hot-remove of virtual NICs anymore. I've adjusted this test so it powers off the VM first, which is not so much an issue as we aren't testing for a hot-remove anyway, but more to make sure that things are consistent in state. * The vApp property tests were missing settings for thin_provisioned and eagerly_scrub, which should be coming from the template data sources. These were failing as a result as the CoreOS OVA does not use thin provisioned disks. * Finally, #421 introduced a change where we were actually failing if we didn't recognize the CD drive type. There are a few we still don't support, namely passthrough CD drives. I've changed this to a debug log message, and a unsetting of all parameters. This should ensure that devices that aren't recognized but need to be changed to a recognized device are properly accounted for in the diff, in addition to removing unrecognized devices in the event no CDROM drive is necessary.
tombuildsstuff
approved these changes
Mar 9, 2018
| // devices, this ensures we don't fail on CDROM device types we don't | ||
| // support right now, such as passthrough devices. We might support these | ||
| // later. | ||
| log.Printf("%s: [DEBUG] Unknown CDROM type %T, clearing all attributes", r, backing) |
There was a problem hiding this comment.
this behaviour should probably also be specified in the docs?
There was a problem hiding this comment.
Done! Added an extra blurb on this one in the docs.
* Added some monospace to some spots that should have had it. * Added a blurb about how we handle drives currently unsupported by Terraform.
ghost
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This does a few things:
something changing in RancherOS that does not allow for the proper
hot-remove of virtual NICs anymore. I've adjusted this test so it
powers off the VM first, which is not so much an issue as we aren't
testing for a hot-remove anyway, but more to make sure that things are
consistent in state.
thin_provisionedandeagerly_scrub, which should be coming from the template data sources.These were failing as a result as the CoreOS OVA does not use thin
provisioned disks.
didn't recognize the CD drive type. There are a few we still don't
support, namely passthrough CD drives. I've changed this to a debug log
message, and a unsetting of all parameters. This should ensure that
devices that aren't recognized but need to be changed to a recognized
device are properly accounted for in the diff, in addition to removing
unrecognized devices in the event no CDROM drive is necessary.