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
Rightly take the host parameter into account while cloning a VM on a cluster #2496
Conversation
@jpillon, VMware has approved your signed contributor license agreement. |
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.
Thanks @jpillon ! Looks good, 2 minor suggestions
simulator/virtual_machine.go
Outdated
@@ -1701,8 +1701,16 @@ func (vm *VirtualMachine) CloneVMTask(ctx *Context, req *types.CloneVM_Task) soa | |||
pool = vm.ResourcePool | |||
} | |||
} | |||
|
|||
DestHost := vm.Runtime.Host |
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.
Non-exported variable names should start with lowercase, can you rename to destHost
?
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.
Commit amended ;-)
simulator/virtual_machine.go
Outdated
DestHost := vm.Runtime.Host | ||
|
||
if req.Spec.Location.Host != nil { | ||
hostRef := req.Spec.Location.Host.Reference() |
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.
req.Spec.Location.Host
is already a pointer, so there's no need for hostRef
here, can do the following instead:
if req.Spec.Location.Host != nil {
destHost = req.Spec.Location.Host
}
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.
Sorry for that, I'm not extremely familiar with Go.
I amended my commit for that (since I also needed to change the commit message)
@jpillon one other request, can you prefix the commit message with
|
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.
Thanks @jpillon
Description
Just set the Dest Host properly in the clone task
Closes: #2495
Type of change
Please mark options that are relevant:
not work as expected)
How Has This Been Tested?
Checklist:
this project