Skip to content
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: Split disk Diff #481

Merged
merged 6 commits into from
Apr 23, 2018

Conversation

bill-rich
Copy link
Contributor

Split virtual disk diff function into validation and normalization. Some
normalization tasks should not be performed on new or unmatched virtual
disks, but validation should be performed on all.

Split virtual disk diff function into validation and normalization. Some
normalization tasks should not be performed on new or unmatched virtual
disks, but validation should be performed on all.
@vancluever
Copy link
Contributor

Hey @bill-rich, the problems that we talked about with the diff validation on new disks is happening because the entire diff validation is happening during the stage where we are comparing resources in state with the current state of configuration, remember. I noticed that ValidateDiff still has some values in it that rely on old state, and the operations are executed one after the other, so I'm not too sure if this will fix things.

Also, I was thinking that NormalizeDiff is probably not the best name for the second function, and possibly ValidateDiff is not necessarily either as while it is accurate it kind of assumes that we are only validating once when we are validating more times. Maybe some different naming is in order that reflects these realities?

We should also throw in some PlanOnly tests to make sure that validation is functional in the stages we are trying to target here (diffs with new disks being added, so either new resources completely or diffs where we are adding disks).

Thanks!

},
{
Config: testAccResourceVSphereVirtualMachineConfigMultiHighBusInsufficientBus(),
ExpectError: regexp.MustCompile("unit_number on disk \"disk1\" too high \\(15\\) - maximum value is 14 with 1 SCSI controller\\(s\\)"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be in the first step or is there a reason why the VM needs to be created first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That one is included to make sure the diff validation is working on updates and when there is already a disk present (disk0). This same case is tested at VM creation at: https://github.com/terraform-providers/terraform-provider-vsphere/blob/8027f5e805e92851893755bffa34be1efb9cb2c0/vsphere/resource_vsphere_virtual_machine_test.go#L362

The two tests added in this commit may be a bit overkill as there isn't a specific past bug that I'm trying to check for, but trying to cover other cases where the behavior is slightly different.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dropped the two update tests, so its just the original TestAccResourceVSphereVirtualMachine_highDiskUnitInsufficientBus again.

@vancluever vancluever added the bug Type: Bug label Apr 20, 2018
Copy link
Contributor

@vancluever vancluever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job @bill-rich, LGTM now!

@bill-rich bill-rich merged commit fe5ee6e into master Apr 23, 2018
@bill-rich bill-rich deleted the b-split-disk-validation-and-diff branch April 23, 2018 18:57
@ghost ghost locked and limited conversation to collaborators Apr 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Type: Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants