Misc features in vcsim #855
Conversation
simulator/snapshot.go
Outdated
| } | ||
| } | ||
|
|
||
| func (v VirtualMachineSnapshot) RevertToSnapshotTask(req *types.RevertToSnapshot_Task) soap.HasFault { |
There was a problem hiding this comment.
Make that a pointer receiver *VirtualMachineSnapshot
simulator/user_directory.go
Outdated
| body := &methods.RetrieveUserGroupsBody{} | ||
| var res []types.BaseUserSearchResult | ||
|
|
||
| var compare func(a, b string) bool |
There was a problem hiding this comment.
Looks like it should also be case insensitive when ExactMatch is false. Maybe:
func equal(name string, search string) bool {
return name == search
}
func match(name string, search string) bool {
return strings.Contains(strings.ToLower(name), search)
} Then:
compare := equal
search := req.SearchStr
if !req.ExactMatch {
compare = match
search = strings.ToLower(search)
}
object/user_directory.go
Outdated
|
|
||
| func NewUserDirectory(c *vim25.Client) *UserDirectory { | ||
| return &UserDirectory{ | ||
| Common: NewCommon(c, *c.ServiceContent.UserDirectory), |
There was a problem hiding this comment.
UserDirectory is also a property of HostConfigManager, so we don't want to hardwire ServiceContent.UserDirectory
object/user_directory.go
Outdated
| } | ||
|
|
||
| func GetUserDirectory(c *vim25.Client) (*UserDirectory, error) { | ||
| if c.ServiceContent.UserDirectory == nil { |
There was a problem hiding this comment.
Is there a case where it would be nil? We only have a few of these Get methods for ServiceContent to cover the cases where ESX is not supported, such as GetExtensionManager and GetCustomFieldsManager.
object/user_directory.go
Outdated
| } | ||
| } | ||
|
|
||
| func (u UserDirectory) RetrieveUserGroups( |
There was a problem hiding this comment.
I think we should just skip the object package wrapper for UserDirectory. As we discussed, there's too much in object/ already, we can just use the types+methods packages directly for calling RetrieveUserGroups.
simulator/snapshot.go
Outdated
| mo.VirtualMachineSnapshot | ||
| } | ||
|
|
||
| func NewVirtualMachineSnapshot( |
There was a problem hiding this comment.
Don't think we need this func, the other simulator.New* funcs are called from more than one place or outside of a soap service method, such as NewServiceInstance(). Here you can just inline the 2 lines within CreateSnapshot.
govc/test/vm.bats
Outdated
| @@ -552,6 +552,71 @@ load test_helper | |||
| assert_success | |||
| } | |||
|
|
|||
| @test "vm.snapshot vcsim" { | |||
There was a problem hiding this comment.
Great to have tests against both real ESX and vcsim. But if they are the same, could you move the code to a function and call it from both?
test_snapshot() {
...
}
@test "vm.snapshot" {
esx_env
test_snapshot
}
@test "vm.snapshot vcsim" {
vcsim_env
test_snapshot
simulator/snapshot.go
Outdated
| return snapshot | ||
| } | ||
|
|
||
| func (v *VirtualMachineSnapshot) AddChild(child types.ManagedObjectReference) { |
There was a problem hiding this comment.
Let's not export this method. Or maybe this could just be inlined instead of having the extra method?
simulator/virtual_machine.go
Outdated
| result = append(result, ss.Snapshot) | ||
| result = append(result, allSnapshotsInTree(ss.ChildSnapshotList)...) | ||
| } | ||
| return result |
There was a problem hiding this comment.
style: add an empty line between the statements (there's a few of these in the PR), we do that in most cases for return too:
git grep -A1 -B2 'return ' -- simulator
simulator/virtual_machine_test.go
Outdated
| t.Fatal(err) | ||
| } | ||
|
|
||
| vm := vms[0] |
There was a problem hiding this comment.
Note that for the test cases where you can use any instead of a type, you can avoid the Finder hassle with the Map.Any function:
vm := object.NewVirtualMachine(c, Map.Any("VirtualMachine").Reference())
anfernee
force-pushed
the
vcsim
branch
2 times, most recently
from
7477320
to
941963a
Compare
govc/test/vm_snapshot.bats
Outdated
|
|
||
| load test_helper | ||
|
|
||
| test_vm_snapshot() { |
There was a problem hiding this comment.
ok to call the file and the func/test names just "snapshot" without the "vm" prefix, since the command doesn't have it either.
govc/test/vm_snapshot.bats
Outdated
|
|
||
| vm=$(new_ttylinux_vm) | ||
|
|
||
| # test_vm_snapshot $vm |
There was a problem hiding this comment.
Is this test not working against ESX or was just taking too long to run? You can use this to run a single test: sstephenson/bats#117
(even nicer with bats.el :)
There was a problem hiding this comment.
I forgot to re-enable that..
govc/test/vm_snapshot.bats
Outdated
| vm=$(new_empty_vm) | ||
|
|
||
| test_vm_snapshot $vm | ||
|
|
There was a problem hiding this comment.
whoops, don't want a blank line there
simulator/user_directory.go
Outdated
|
|
||
| for _, ug := range u.userGroup { | ||
| if req.FindUsers && !ug.Group || req.FindGroups && ug.Group { | ||
| compare := compareFunc(req.SearchStr, req.ExactMatch) |
There was a problem hiding this comment.
Looks like you could move this outside of the for loop, so it only needs to be called once.
| return body | ||
| } | ||
|
|
||
| func compareFunc(compared string, exactly bool) func(string) bool { |
Fixes #819 #821 (I assume it will fix those, not exactly sure because of lack of context)