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

Fix for error handling on set command in admin cli. #610

Merged
merged 1 commit into from Oct 14, 2016
Merged

Conversation

govint
Copy link
Contributor

@govint govint commented Oct 5, 2016

Modified vmdk_ops.py set_vol_opts() to not raise validation errors, surrounding code doesn't raise errors and instead log the errors as warnings. This was the only case where an exception was being raised and un-handled. Plus ValidationError is specific to vmdk_ops.py.

Test is already there for vmdkops_admin.py and vmdk_ops.py for access option.

Code now prints the error and the usage:

vmdkops_admin.py set --options="acces=read-write" --volume=testvol-11@bigone

WARNING:root:Invalid options: ['acces']
Options that can be edited: ['attach-as', 'access']
Failed to update acces=read-write for testvol-11@bigone.

vmdkops_admin.py set --options="access=read-write" --volume=testvol-11@bigone
Successfully updated settings for : testvol-11@bigone

@govint
Copy link
Contributor Author

govint commented Oct 5, 2016

Fixes #609

print('Successfully updated settings for : {0}'.format(args.volume))
else:
print('Failed to update {0} for {1}.'.format(args.options, args.volume))
except Exception as ex:
print('Failed to update {0} for {1}.'.format(args.options, args.volume))
Copy link
Contributor

Choose a reason for hiding this comment

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

print ex.message so user knows cause of failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on this.
WARNING:root:Invalid options: ['acces'] seems to be showing in the log, not sysout. Or is it in sysout ?
Can you clarify the exact print happening on typos here?

@govint
Copy link
Contributor Author

govint commented Oct 7, 2016

The error messages are all on the command line. I'll print the exception
message if any.

On Fri, Oct 7, 2016 at 2:16 AM, Mark Sterin notifications@github.com
wrote:

@msterin commented on this pull request.

In esx_service/cli/vmdkops_admin.py
#610:

@@ -563,10 +563,13 @@ def status(args):

def set_vol_opts(args):

  • set_ok = vmdk_ops.set_vol_opts(args.volume, args.options)
  • if set_ok:
  •    print('Successfully updated settings for : {0}'.format(args.volume))
    
  • else:
  • try:
  •    set_ok = vmdk_ops.set_vol_opts(args.volume, args.options)
    
  •    if set_ok:
    
  •       print('Successfully updated settings for : {0}'.format(args.volume))
    
  •    else:
    
  •       print('Failed to update {0} for {1}.'.format(args.options, args.volume))
    
  • except Exception as ex:
    print('Failed to update {0} for {1}.'.format(args.options, args.volume))

+1 on this.
WARNING:root:Invalid options: ['acces'] seems to be showing in the log,
not sysout. Or is it in sysout ?
Can you clarify the exact print happening on typos here?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#610, or mute the
thread
https://github.com/notifications/unsubscribe-auth/APHseHcONkwiUdXaZbLru42UwhjonNW8ks5qxV4SgaJpZM4KOrBS
.

@msterin
Copy link
Contributor

msterin commented Oct 12, 2016

@govint - can you please add the current error message (as printed by Admin CLI on this error, and some other error for comparison) so we can LGTM and merge this one ?

@govint
Copy link
Contributor Author

govint commented Oct 14, 2016

Updated the change to ensure that exceptions are caught and the exception message is printed

  1. Exception handled and error printed on stdout.

[root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py set --options="acces=read-write" --volume=testvol-11@bigone
Failed to update acces=read-write for testvol-11@bigone - "Invalid options: ['acces'] \nOptions that can be edited: ['attach-as', 'access']".

  1. When the correct option is used
    [root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py set --options="access=read-write" --volume=testvol-11@bigone
    Successfully updated settings for : testvol-11@bigone
  2. A missing volume
    [root@localhost:~] /usr/lib/vmware/vmdkops/bin/vmdkops_admin.py set --options="access=read-write" --volume=testvol-110@bigone
    WARNING:root:Volume testvol-110 not found.
    Failed to update access=read-write for testvol-110@bigone.

@msterin msterin merged commit f028ad2 into master Oct 14, 2016
@msterin
Copy link
Contributor

msterin commented Oct 15, 2016

Fixes #609

@msterin msterin deleted the fix-acli branch October 22, 2016 00:42
brunotm added a commit to brunotm/docker-volume-vsphere that referenced this pull request Oct 26, 2016
* master: (25 commits)
  Update new ESX IP
  added forgotten .so file
  Install sqlite3 py libs on ESX and load for Python2
  Added py code and binaries for sqlite3 python libs
  Update drone security
  Removed accidental .pyc files
  Handle byte to string conversions for status command.
  Auth configuration and operation admission check (Auth.liping) (vmware-archive#603)
  Revert "Cli auth.liping"
  Cli auth.liping (vmware-archive#640)
  Handle missing or invalid fs type on mount. (vmware-archive#639)
  Updated Admin CLI commands to support tenants. (vmware-archive#620)
  Workaround older versions of e2fsprogs (vmware-archive#631)
  Add auth proposal
  Made handing of missing metafile less harsh. (vmware-archive#627)
  Fixed ACLs in payload bin dir (vmware-archive#624)
  Fixed error handling for set command. (vmware-archive#610)
  Use new error variables when rolling back volume creation to avoid nil reassignment. (vmware-archive#617)
  Change wording
  Fix broken link
  ...
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.

None yet

4 participants