Support "--no" option in aggregate set

Supporting "--no-property" option will apply user a convenience
way to clean all properties of aggregate in a short command,
and this kind of behavior is the recommended way to devref.
The patch add "--no-property" option in "aggregate set" command,
and update related test cases and devref document.

Change-Id: I7614a23c0db05144562330dc600dbab7d003d5d8
Implements: blueprint support-no-property-in-aggregate

Author: kiwik

doc/source/command-objects/aggregate.rst

@@ -116,6 +116,7 @@ Set aggregate properties
         [--name <new-name>]
         [--zone <availability-zone>]
         [--property <key=value> [...] ]
+        [--no-property]
         <aggregate>
 
 .. option:: --name <name>
@@ -131,6 +132,12 @@ Set aggregate properties
     Property to set on :ref:`\<aggregate\> <aggregate_set-aggregate>`
     (repeat option to set multiple properties)
 
+.. option:: --no-property
+
+    Remove all properties from :ref:`\<aggregate\> <aggregate_set-aggregate>`
+    (specify both --property and --no-property to
+    overwrite the current properties)
+
 .. _aggregate_set-aggregate:
 .. describe:: <aggregate>
 

openstackclient/compute/v2/aggregate.py

@@ -248,6 +248,14 @@ def get_parser(self, prog_name):
             help=_("Property to set on <aggregate> "
                    "(repeat option to set multiple properties)")
         )
+        parser.add_argument(
+            "--no-property",
+            dest="no_property",
+            action="store_true",
+            help=_("Remove all properties from <aggregate> "
+                   "(specify both --property and --no-property to "
+                   "overwrite the current properties)"),
+        )
         return parser
 
     def take_action(self, parsed_args):
@@ -269,10 +277,23 @@ def take_action(self, parsed_args):
                 kwargs
             )
 
+        set_property = {}
+        if parsed_args.no_property:
+            # NOTE(RuiChen): "availability_zone" is removed from response of
+            #                aggregate show and create commands, don't see it
+            #                anywhere, so pop it, avoid the unexpected server
+            #                exception(can't unset the availability zone from
+            #                aggregate metadata in nova).
+            set_property.update({key: None
+                                 for key in aggregate.metadata.keys()
+                                 if key != 'availability_zone'})
         if parsed_args.property:
+            set_property.update(parsed_args.property)
+
+        if set_property:
             compute_client.aggregates.set_metadata(
                 aggregate,
-                parsed_args.property
+                set_property
             )
 
 
@@ -326,7 +347,6 @@ def get_parser(self, prog_name):
             "--property",
             metavar="<key>",
             action='append',
-            required=True,
             help=_("Property to remove from aggregate "
                    "(repeat option to remove multiple properties)")
         )
@@ -338,6 +358,9 @@ def take_action(self, parsed_args):
             compute_client.aggregates,
             parsed_args.aggregate)
 
-        unset_property = {key: None for key in parsed_args.property}
-        compute_client.aggregates.set_metadata(aggregate,
-                                               unset_property)
+        unset_property = {}
+        if parsed_args.property:
+            unset_property.update({key: None for key in parsed_args.property})
+        if unset_property:
+            compute_client.aggregates.set_metadata(aggregate,
+                                                   unset_property)

openstackclient/tests/unit/compute/v2/fakes.py

@@ -83,6 +83,7 @@ def create_one_aggregate(attrs=None):
             "id": "aggregate-id-" + uuid.uuid4().hex,
             "metadata": {
                 "availability_zone": "ag_zone",
+                "key1": "value1",
             }
         }
 

openstackclient/tests/unit/compute/v2/test_aggregate.py

@@ -21,7 +21,6 @@
 
 from openstackclient.compute.v2 import aggregate
 from openstackclient.tests.unit.compute.v2 import fakes as compute_fakes
-from openstackclient.tests.unit import utils as tests_utils
 
 
 class TestAggregate(compute_fakes.TestComputev2):
@@ -235,7 +234,8 @@ class TestAggregateList(TestAggregate):
         TestAggregate.fake_ag.id,
         TestAggregate.fake_ag.name,
         TestAggregate.fake_ag.availability_zone,
-        {},
+        {key: value for key, value in TestAggregate.fake_ag.metadata.items()
+         if key != 'availability_zone'},
     ), )
 
     def setUp(self):
@@ -371,6 +371,62 @@ def test_aggregate_set_with_property(self):
             self.fake_ag, parsed_args.property)
         self.assertIsNone(result)
 
+    def test_aggregate_set_with_no_property_and_property(self):
+        arglist = [
+            '--no-property',
+            '--property', 'key2=value2',
+            'ag1',
+        ]
+        verifylist = [
+            ('no_property', True),
+            ('property', {'key2': 'value2'}),
+            ('aggregate', 'ag1'),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        result = self.cmd.take_action(parsed_args)
+        self.aggregate_mock.get.assert_called_once_with(parsed_args.aggregate)
+        self.assertNotCalled(self.aggregate_mock.update)
+        self.aggregate_mock.set_metadata.assert_called_once_with(
+            self.fake_ag, {'key1': None, 'key2': 'value2'})
+        self.assertIsNone(result)
+
+    def test_aggregate_set_with_no_property(self):
+        arglist = [
+            '--no-property',
+            'ag1',
+        ]
+        verifylist = [
+            ('no_property', True),
+            ('aggregate', 'ag1'),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        result = self.cmd.take_action(parsed_args)
+        self.aggregate_mock.get.assert_called_once_with(parsed_args.aggregate)
+        self.assertNotCalled(self.aggregate_mock.update)
+        self.aggregate_mock.set_metadata.assert_called_once_with(
+            self.fake_ag, {'key1': None})
+        self.assertIsNone(result)
+
+    def test_aggregate_set_with_zone_and_no_property(self):
+        arglist = [
+            '--zone', 'new_zone',
+            '--no-property',
+            'ag1',
+        ]
+        verifylist = [
+            ('zone', 'new_zone'),
+            ('no_property', True),
+            ('aggregate', 'ag1'),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        result = self.cmd.take_action(parsed_args)
+        self.aggregate_mock.get.assert_called_once_with(parsed_args.aggregate)
+        self.aggregate_mock.update.assert_called_once_with(
+            self.fake_ag, {'availability_zone': parsed_args.zone})
+        self.aggregate_mock.set_metadata.assert_called_once_with(
+            self.fake_ag, {'key1': None})
+        self.assertIsNone(result)
+
 
 class TestAggregateShow(TestAggregate):
 
@@ -387,7 +443,10 @@ class TestAggregateShow(TestAggregate):
         TestAggregate.fake_ag.hosts,
         TestAggregate.fake_ag.id,
         TestAggregate.fake_ag.name,
-        '',
+        utils.format_dict(
+            {key: value
+             for key, value in TestAggregate.fake_ag.metadata.items()
+             if key != 'availability_zone'}),
     )
 
     def setUp(self):
@@ -435,13 +494,32 @@ def test_aggregate_unset(self):
             self.fake_ag, {'unset_key': None})
         self.assertIsNone(result)
 
-    def test_aggregate_unset_no_property(self):
+    def test_aggregate_unset_multiple_properties(self):
         arglist = [
+            '--property', 'unset_key1',
+            '--property', 'unset_key2',
             'ag1',
         ]
-        verifylist = None
-        self.assertRaises(tests_utils.ParserException,
-                          self.check_parser,
-                          self.cmd,
-                          arglist,
-                          verifylist)
+        verifylist = [
+            ('property', ['unset_key1', 'unset_key2']),
+            ('aggregate', 'ag1'),
+        ]
+
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        result = self.cmd.take_action(parsed_args)
+        self.aggregate_mock.set_metadata.assert_called_once_with(
+            self.fake_ag, {'unset_key1': None, 'unset_key2': None})
+        self.assertIsNone(result)
+
+    def test_aggregate_unset_no_option(self):
+        arglist = [
+            'ag1',
+        ]
+        verifylist = [
+            ('property', None),
+            ('aggregate', 'ag1'),
+        ]
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        result = self.cmd.take_action(parsed_args)
+        self.assertNotCalled(self.aggregate_mock.set_metadata)
+        self.assertIsNone(result)

releasenotes/notes/support-no-property-in-aggregate-b74a42e00a65d14a.yaml

@@ -0,0 +1,7 @@
+---
+features:
+  - |
+    Add ``--no-property`` option in ``aggregate set``.
+    Supporting ``--no-property`` option will apply user a convenience way to
+    clean all properties of aggregate in a short command.
+    [Blueprint `support-no-property-in-aggregate <https://blueprints.launchpad.net/python-openstackclient/+spec/support-no-property-in-aggregate>`_]