-
Notifications
You must be signed in to change notification settings - Fork 95
Fix issue #1188: vmgroup is not removable when vms are added from both ESXs #1231
Conversation
…h ESXs (See original PR #1214 for the details of review comments)
esx_service/utils/test_utils.py
Outdated
auth_api._tenant_vm_rm(name=name, | ||
vm_list=vm_names) | ||
auth_api._tenant_rm(name=name, | ||
remove_volumes=True) |
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.
nit: no new line at the end.
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.
Addressed.
@pshahzeb @shuklanirdesh82 The CI test has passed: https://ci.vmware.run/vmware/docker-volume-vsphere/95 |
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.
LGTM
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.
L844 and L755: to keep it consistent.
Your proposed changes looks good to me. some minor comments for consistency.
error_info = auth_api._tenant_vm_rm(name=self.tenant1_new_name, | ||
vm_list=vm_list) | ||
self.assertEqual(None, error_info) | ||
|
||
error_info = auth_api._tenant_rm( |
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.
can we use test_utils.cleanup_tenant
to keep it consistent?
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.
Sure, will address.
Thanks @shaominchen for addressing quickly! |
Note: This PR was created for a clean commit history. See original PR #1214 for test result and previous review comments)