-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
provider/vsphere: govmomi v0.6.1 update and fixes #6479
Conversation
LOL - we did the same stuff ... ... I will review. |
This is marked as WIP?? |
@@ -1692,7 +1684,7 @@ func (vm *virtualMachine) deployVirtualMachine(c *govmomi.Client) error { | |||
for _, dvc := range devices { | |||
// Issue 3559/3560: Delete all ethernet devices to add the correct ones later | |||
if devices.Type(dvc) == "ethernet" { | |||
err := newVM.RemoveDevice(context.TODO(), dvc) | |||
err := newVM.RemoveDevice(context.TODO(), false, dvc) |
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.
We should consider setting false
as a parameter. No change needed at this point.
@chrislovecnm WIP was because I pushed it without much testing (had to leave the office) and I wasn't completely sure about the function changes. |
Ideally here someone with a vsphere setup would run the acceptance tests against this branch and ensure that there are no unexpected functional changes here - I just merged a fix from @thetuxkeeper into master, so it's possible it will want further rebasing. Thanks all! |
@jen20 yes I will, just need someone with better golang kungfu to look at the casting that we are doing. |
@@ -423,7 +423,7 @@ func resourceVSphereVirtualMachineUpdate(d *schema.ResourceData, meta interface{ | |||
configSpec := types.VirtualMachineConfigSpec{} | |||
|
|||
if d.HasChange("vcpu") { | |||
configSpec.NumCPUs = d.Get("vcpu").(int) | |||
configSpec.NumCPUs = int32(d.Get("vcpu").(int)) |
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.
@jen20 the cast to int32
is new ... opinion? I know you go fu is strong :)
I'll let the acceptance tests run against this branch with #6480 merged locally (it will fail without it). |
Both of use need to.... lol |
We have something odd going on: --- FAIL: TestAccVSphereVirtualMachine_basic (163.41s)
testing.go:172: Step 0 error: Check failed: Check 5/9 error: vsphere_virtual_machine.foo: Attribute 'memory_reservation' expected "4096", got "0"
FAIL
exit status 1
FAIL github.com/hashicorp/terraform/builtin/providers/vsphere 163.431s I may not have time to debug today ... But this needs to get resolved. @thetuxkeeper what is you setup? I am on vSphere 5.5 @markpeek are you able to run the integration tests? I may have environment issues. |
@thetuxkeeper do I have your email address? Other topic |
@chrislovecnm the error is fixed with #6480 |
... Yah @markpeek just helped me out with that one |
Looking better ... as @markpeek said we need to document how to run the darn integration tests better. Maybe example bash script or something ... |
@thetuxkeeper the only test that is failing is |
OS customization doesn't work for me, so even |
@thetuxkeeper you have open-vm-tools installed? |
You will want the latest |
|
Most work, but some fail. Will look at it at friday (tomorrow we have a holiday and it's already late here)
Have to check
|
@thetuxkeeper were are we on this? @stack72 has mentioned that 0.7 will not arrive for a bit, and he is recommending that we push it. Thoughts? |
@chrislovecnm : With #6656 my test run looks good.
|
Merging as requested :) Thanks all! |
@stack72 thanks Paul |
@stack72 Thanks! :) |
* vendor: Update dependency vmware/govmomi * fixed data types * fixed RemoveDevice * fixed CreateDisk usage
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
I used PR #6472 (thanks @catsby :) ), rebased it to master and started to fix some issues:
RemoveDevice
: "keepFiles" boolean was added => false for network devices (should be relevant for disks only)CreateDisk
: not additional parameter for the datastore.newDiskPath
logic removed ("empty" check is in govmomi now)cc @chrislovecnm