Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Returning proper exit code in cases where vmdkops admin command fails. #1543

Merged
merged 1 commit into from
Jul 6, 2017

Conversation

pshahzeb
Copy link
Contributor

@pshahzeb pshahzeb commented Jul 6, 2017

This PR is about returning proper exit code to indicate error when command fails to maintain consistency.
Removed redundant helper to print the error message and return it.

Fixes #1522

Testing:
test-e2e passes

2017/07/06 00:38:40 Destroying volume [vsanVol_volume_1016373@vsanDatastore]
2017/07/06 00:38:42 Removing policy [validPolicy] on esx [10.192.248.54]
OK: 28 passed
--- PASS: Test (1411.95s)
PASS
ok  	github.com/vmware/docker-volume-vsphere/tests/e2e	1411.957s
make: Leaving directory `/go/src/github.com/vmware/docker-volume-vsphere/vmdk_plugin'

Manual testing (Sample):

[root@sc2-rdops-vm07-dhcp-248-54:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py policy ls
Policy Name  Policy Content  Active
-----------  --------------  ------

[root@sc2-rdops-vm07-dhcp-248-54:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py policy rm --name IDONTEXIST
Error: IDONTEXIST does not exist
[root@sc2-rdops-vm07-dhcp-248-54:~] echo $?
2
[root@sc2-rdops-vm07-dhcp-248-54:~]

1. Maintaining unambiguity through returning proper exit code when command fails along with message print.
2. Removed redundant helper to print the error message and return it.

Fixes vmware-archive#1522
@pshahzeb pshahzeb self-assigned this Jul 6, 2017
Copy link
Contributor

@govint govint left a comment

Choose a reason for hiding this comment

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

Looks good.

@@ -958,7 +954,7 @@ def tenant_create(args):
privileges=[])

if error_info:
return operation_fail(error_info.msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is operation_fail still being used? If not, we should remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I have removed it. See line 902

Copy link
Contributor

@shaominchen shaominchen left a comment

Choose a reason for hiding this comment

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

LGTM.

@pshahzeb pshahzeb merged commit 6f4d7d7 into vmware-archive:master Jul 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants