Tweak volume commands and add k=v argparse action

Basic cleanups:
* change metadata to property
* add new KeyValueAction to parse the property options
* multiple properties can be set using multiple --property args
* consistent formatting
* do lookups for volume args

Change-Id: Ib6c43f01ad46b395aee8c61e886f42e2a5f5573e

4 files changed, 223 insertions, 58 deletions

diff --git a/openstackclient/common/parseractions.py b/openstackclient/common/parseractions.py
new file mode 100644
index 00000000..f111c264
--- /dev/null
+++ b/openstackclient/common/parseractions.py
@@ -0,0 +1,34 @@
+#   Copyright 2013 OpenStack Foundation
+#
+#   Licensed under the Apache License, Version 2.0 (the "License"); you may
+#   not use this file except in compliance with the License. You may obtain
+#   a copy of the License at
+#
+#        http://www.apache.org/licenses/LICENSE-2.0
+#
+#   Unless required by applicable law or agreed to in writing, software
+#   distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#   WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#   License for the specific language governing permissions and limitations
+#   under the License.
+#
+
+"""argparse Custom Actions"""
+
+import argparse
+
+
+class KeyValueAction(argparse.Action):
+    """A custom action to parse arguments as key=value pairs.
+    Ensures that dest is a dict
+    """
+    def __call__(self, parser, namespace, values, option_string=None):
+        # Make sure we have an empty dict rather than None
+        if getattr(namespace, self.dest, None) is None:
+            setattr(namespace, self.dest, {})
+
+        # Add value if an assignment else remove it
+        if '=' in values:
+            getattr(namespace, self.dest, {}).update([values.split('=', 1)])
+        else:
+            getattr(namespace, self.dest, {}).pop(values, None)
diff --git a/openstackclient/volume/v1/type.py b/openstackclient/volume/v1/type.py
index 9d79f8b3..e146ee3f 100644
--- a/openstackclient/volume/v1/type.py
+++ b/openstackclient/volume/v1/type.py
@@ -21,6 +21,7 @@ from cliff import command
 from cliff import lister
 from cliff import show
 
+from openstackclient.common import parseractions
 from openstackclient.common import utils
 
 
@@ -118,21 +119,22 @@ class SetVolumeType(command.Command):
             help='Volume type name or ID to update',
         )
         parser.add_argument(
-            'meta_data',
+            '--property',
             metavar='<key=value>',
-            help='meta-data to add to volume type',
+            action=parseractions.KeyValueAction,
+            help='Property to add/change for this volume type '
+                 '(repeat option to set multiple properties)',
         )
         return parser
 
     def take_action(self, parsed_args):
         self.log.debug('take_action(%s)' % parsed_args)
-
-        meta = dict(v.split('=') for v in parsed_args.meta_data.split(' '))
         volume_client = self.app.client_manager.volume
         volume_type = utils.find_resource(
             volume_client.volume_types, parsed_args.volume_type)
 
-        volume_type.set_keys(meta)
+        if parsed_args.property:
+            volume_type.set_keys(parsed_args.property)
 
         return
 
@@ -148,12 +150,15 @@ class UnsetVolumeType(command.Command):
         parser.add_argument(
             'volume_type',
             metavar='<volume-type>',
-            help='Type ID or name to update',
+            help='Type ID or name to remove',
         )
         parser.add_argument(
-            'meta_data',
+            '--property',
             metavar='<key>',
-            help='meta-data to remove from volume type (key only)',
+            action='append',
+            default=[],
+            help='Property key to remove from volume '
+                 '(repeat option to remove multiple properties)',
         )
         return parser
 
@@ -161,12 +166,17 @@ class UnsetVolumeType(command.Command):
         self.log.debug('take_action(%s)' % parsed_args)
         volume_client = self.app.client_manager.volume
         volume_type = utils.find_resource(
-            volume_client.volume_types, parsed_args.volume_type)
-
-        key_list = []
-        key_list.append(parsed_args.meta_data)
-        volume_type.unset_keys(key_list)
+            volume_client.volume_types,
+            parsed_args.volume_type,
+        )
 
+        if parsed_args.property:
+            volume_client.volumes.delete_metadata(
+                volume_type.id,
+                parsed_args.property,
+            )
+        else:
+            self.app.log.error("No changes requested\n")
         return
 
 
diff --git a/openstackclient/volume/v1/volume.py b/openstackclient/volume/v1/volume.py
index f9641c54..43253c40 100644
--- a/openstackclient/volume/v1/volume.py
+++ b/openstackclient/volume/v1/volume.py
@@ -16,12 +16,12 @@
 """Volume v1 Volume action implementations"""
 
 import logging
-import sys
 
 from cliff import command
 from cliff import lister
 from cliff import show
 
+from openstackclient.common import parseractions
 from openstackclient.common import utils
 
 
@@ -63,32 +63,34 @@ class CreateVolume(show.ShowOne):
         parser.add_argument(
             '--user-id',
             metavar='<user-id>',
-            help='User id derived from context',
+            help='Override user id derived from context (admin only)',
         )
         parser.add_argument(
             '--project-id',
             metavar='<project-id>',
-            help='Project id derived from context',
+            help='Override project id derived from context (admin only)',
         )
         parser.add_argument(
             '--availability-zone',
             metavar='<availability-zone>',
-            help='Availability Zone to use',
+            help='Availability zone to use',
         )
         parser.add_argument(
             '--property',
             metavar='<key=value>',
-            help='Optional property to set on volume creation',
+            action=parseractions.KeyValueAction,
+            help='Property to store for this volume '
+                 '(repeat option to set multiple properties)',
         )
         parser.add_argument(
-            '--image-ref',
-            metavar='<image-ref>',
-            help='reference to an image stored in glance',
+            '--image',
+            metavar='<image>',
+            help='Reference to a stored image',
         )
         parser.add_argument(
-            '--source-volid',
-            metavar='<source-volid>',
-            help='ID of source volume to clone from',
+            '--source',
+            metavar='<volume>',
+            help='Source for volume clone',
         )
 
         return parser
@@ -98,22 +100,25 @@ class CreateVolume(show.ShowOne):
 
         volume_client = self.app.client_manager.volume
 
-        meta = None
-        if parsed_args.meta_data:
-            meta = dict(v.split('=') for v in parsed_args.meta_data.split(' '))
+        source_volume = None
+        if parsed_args.source:
+            source_volume = utils.find_resource(
+                volume_client.volumes,
+                parsed_args.source,
+            ).id
 
         volume = volume_client.volumes.create(
             parsed_args.size,
             parsed_args.snapshot_id,
-            parsed_args.source_volid,
+            source_volume,
             parsed_args.name,
             parsed_args.description,
             parsed_args.volume_type,
             parsed_args.user_id,
             parsed_args.project_id,
             parsed_args.availability_zone,
-            meta,
-            parsed_args.image_ref
+            parsed_args.property,
+            parsed_args.image
         )
 
         return zip(*sorted(volume._info.iteritems()))
@@ -175,13 +180,13 @@ class ListVolume(lister.Lister):
             '--all-tenants',
             action='store_true',
             default=False,
-            help='Display information from all tenants (Admin-only)',
+            help='Display information from all tenants (admin only)',
         )
         parser.add_argument(
             '--long',
             action='store_true',
             default=False,
-            help='Display meta-data',
+            help='Display properties',
         )
         return parser
 
@@ -221,19 +226,25 @@ class SetVolume(command.Command):
         parser.add_argument(
             'volume',
             metavar='<volume>',
-            help='Name or ID of volume to change')
+            help='Name or ID of volume to change',
+        )
         parser.add_argument(
             '--name',
-            metavar='<new-volume-name>',
-            help='New volume name')
+            metavar='<new-name>',
+            help='New volume name',
+        )
         parser.add_argument(
             '--description',
-            metavar='<volume-description>',
-            help='New volume description')
+            metavar='<new-description>',
+            help='New volume description',
+        )
         parser.add_argument(
-            '--meta-data',
+            '--property',
             metavar='<key=value>',
-            help='meta-data to add to volume')
+            action=parseractions.KeyValueAction,
+            help='Property to add/change for this volume '
+                 '(repeat option to set multiple properties)',
+        )
         return parser
 
     def take_action(self, parsed_args):
@@ -241,21 +252,22 @@ class SetVolume(command.Command):
         volume_client = self.app.client_manager.volume
         volume = utils.find_resource(volume_client.volumes, parsed_args.volume)
 
-        meta = None
         if parsed_args.property:
-            meta = dict(v.split('=') for v in parsed_args.property.split(' '))
-            volume_client.volumes.set_metadata(volume.id, meta)
+            print "property: %s" % parsed_args.property
+            volume_client.volumes.set_metadata(volume.id, parsed_args.property)
 
         kwargs = {}
         if parsed_args.name:
             kwargs['display_name'] = parsed_args.name
         if parsed_args.description:
             kwargs['display_description'] = parsed_args.description
+        if kwargs:
+            print "kwargs: %s" % kwargs
+            volume_client.volumes.update(volume.id, **kwargs)
+
+        if not kwargs and not parsed_args.property:
+            self.app.log.error("No changes requested\n")
 
-        if not kwargs and not meta:
-            sys.stdout.write("Volume not updated, no arguments present \n")
-            return
-        volume_client.volumes.update(volume.id, **kwargs)
         return
 
 
@@ -270,7 +282,8 @@ class ShowVolume(show.ShowOne):
         parser.add_argument(
             'volume',
             metavar='<volume>',
-            help='Name or ID of volume to display')
+            help='Name or ID of volume to display',
+        )
         return parser
 
     def take_action(self, parsed_args):
@@ -292,11 +305,16 @@ class UnsetVolume(command.Command):
         parser.add_argument(
             'volume',
             metavar='<volume>',
-            help='Name or ID of volume to change')
+            help='Name or ID of volume to change',
+        )
         parser.add_argument(
-            '--meta-data',
+            '--property',
             metavar='<key>',
-            help='meta-data to remove from volume (key only)')
+            action='append',
+            default=[],
+            help='Property key to remove from volume '
+                 '(repeat to set multiple values)',
+        )
         return parser
 
     def take_action(self, parsed_args):
@@ -305,14 +323,13 @@ class UnsetVolume(command.Command):
         volume = utils.find_resource(
             volume_client.volumes, parsed_args.volume)
 
-        if not parsed_args.meta_data:
-            sys.stdout.write("Volume not updated, no arguments present \n")
-            return
-
-        key_list = []
-        key_list.append(parsed_args.meta_data)
-        volume_client.volumes.delete_metadata(volume.id, key_list)
-
+        if parsed_args.property:
+            volume_client.volumes.delete_metadata(
+                volume.id,
+                parsed_args.property,
+            )
+        else:
+            self.app.log.error("No changes requested\n")
         return
 
 
diff --git a/tests/common/test_parseractions.py b/tests/common/test_parseractions.py
new file mode 100644
index 00000000..f48c4d72
--- /dev/null
+++ b/tests/common/test_parseractions.py
@@ -0,0 +1,104 @@
+#   Copyright 2012-2013 OpenStack Foundation
+#
+#   Licensed under the Apache License, Version 2.0 (the "License"); you may
+#   not use this file except in compliance with the License. You may obtain
+#   a copy of the License at
+#
+#        http://www.apache.org/licenses/LICENSE-2.0
+#
+#   Unless required by applicable law or agreed to in writing, software
+#   distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#   WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#   License for the specific language governing permissions and limitations
+#   under the License.
+#
+
+import argparse
+
+from openstackclient.common import parseractions
+from tests import utils
+
+
+class TestKeyValueAction(utils.TestCase):
+    def test_good_values(self):
+        parser = argparse.ArgumentParser()
+
+        # Set up our typical usage
+        parser.add_argument(
+            '--property',
+            metavar='<key=value>',
+            action=parseractions.KeyValueAction,
+            help='Property to store for this volume '
+                 '(repeat option to set multiple properties)',
+        )
+
+        results = parser.parse_args([
+            '--property', 'red=',
+            '--property', 'green=100%',
+            '--property', 'blue=50%',
+        ])
+
+        actual = getattr(results, 'property', {})
+        # All should pass through unmolested
+        expect = {'red': '', 'green': '100%', 'blue': '50%'}
+        self.assertDictEqual(expect, actual)
+
+    def test_default_values(self):
+        parser = argparse.ArgumentParser()
+
+        # Set up our typical usage
+        parser.add_argument(
+            '--property',
+            metavar='<key=value>',
+            action=parseractions.KeyValueAction,
+            default={'green': '20%', 'format': '#rgb'},
+            help='Property to store for this volume '
+                 '(repeat option to set multiple properties)',
+        )
+
+        results = parser.parse_args([
+            '--property', 'red=',
+            '--property', 'green=100%',
+            '--property', 'blue=50%',
+        ])
+
+        actual = getattr(results, 'property', {})
+        # Verify green default is changed, format default is unchanged
+        expect = {'red': '', 'green': '100%', 'blue': '50%', 'format': '#rgb'}
+        self.assertDictEqual(expect, actual)
+
+    def test_error_values(self):
+        parser = argparse.ArgumentParser()
+
+        # Set up our typical usage
+        parser.add_argument(
+            '--property',
+            metavar='<key=value>',
+            action=parseractions.KeyValueAction,
+            default={'green': '20%', 'blue': '40%'},
+            help='Property to store for this volume '
+                 '(repeat option to set multiple properties)',
+        )
+
+        results = parser.parse_args([
+            '--property', 'red',
+            '--property', 'green=100%',
+            '--property', 'blue',
+        ])
+
+        failhere = None
+        actual = getattr(results, 'property', {})
+        # Verify non-existant red key
+        try:
+            failhere = actual['red']
+        except Exception as e:
+            self.assertTrue(type(e) == KeyError)
+        # Verify removal of blue key
+        try:
+            failhere = actual['blue']
+        except Exception as e:
+            self.assertTrue(type(e) == KeyError)
+        # There should be no red or blue
+        expect = {'green': '100%'}
+        self.assertDictEqual(expect, actual)
+        self.assertEqual(failhere, None)