From 762bf69c3657d327ce694714c8a900ae36ebd768 Mon Sep 17 00:00:00 2001 From: neetu Date: Thu, 5 Jun 2014 14:39:28 -0500 Subject: [PATCH] Adding cornercases for set_metadata Aggregate_set_metadata leaves users confused because it currently does not check for duplicate key=value parameters for addition or a missing key for deletion. All that user sees is that "metadata has been updated" more details in bug1292572 This patch checks for above mentioned corner cases and informs users when and why there was no change made in above corner cases Adding better testcases also. Closes-Bug: #1292572 Change-Id: I820d793ac44680295df8b2a3f58a0834f7019875 --- novaclient/tests/v1_1/fakes.py | 16 +++++++++++++++ novaclient/tests/v1_1/test_shell.py | 30 ++++++++++++++++++++--------- novaclient/tests/v3/test_shell.py | 30 ++++++++++++++++++++--------- novaclient/v1_1/shell.py | 8 ++++++++ novaclient/v3/shell.py | 8 ++++++++ 5 files changed, 74 insertions(+), 18 deletions(-) diff --git a/novaclient/tests/v1_1/fakes.py b/novaclient/tests/v1_1/fakes.py index 35fd44d60..6b418e0c4 100644 --- a/novaclient/tests/v1_1/fakes.py +++ b/novaclient/tests/v1_1/fakes.py @@ -1450,15 +1450,25 @@ class FakeHTTPClient(base_client.HTTPClient): {'id': '2', 'name': 'test2', 'availability_zone': 'nova1'}, + {'id': '3', + 'name': 'test3', + 'metadata': {'test': "dup", "none_key": "Nine"}}, ]}) def _return_aggregate(self): r = {'aggregate': self.get_os_aggregates()[2]['aggregates'][0]} return (200, {}, r) + def _return_aggregate_3(self): + r = {'aggregate': self.get_os_aggregates()[2]['aggregates'][2]} + return (200, {}, r) + def get_os_aggregates_1(self, **kw): return self._return_aggregate() + def get_os_aggregates_3(self, **kw): + return self._return_aggregate_3() + def post_os_aggregates(self, body, **kw): return self._return_aggregate() @@ -1468,12 +1478,18 @@ class FakeHTTPClient(base_client.HTTPClient): def put_os_aggregates_2(self, body, **kw): return self._return_aggregate() + def put_os_aggregates_3(self, body, **kw): + return self._return_aggregate_3() + def post_os_aggregates_1_action(self, body, **kw): return self._return_aggregate() def post_os_aggregates_2_action(self, body, **kw): return self._return_aggregate() + def post_os_aggregates_3_action(self, body, **kw): + return self._return_aggregate_3() + def delete_os_aggregates_1(self, **kw): return (202, {}, None) diff --git a/novaclient/tests/v1_1/test_shell.py b/novaclient/tests/v1_1/test_shell.py index 38c721552..67600350b 100644 --- a/novaclient/tests/v1_1/test_shell.py +++ b/novaclient/tests/v1_1/test_shell.py @@ -1256,17 +1256,29 @@ class ShellTest(utils.TestCase): self.assert_called('PUT', '/os-aggregates/1', body, pos=-2) self.assert_called('GET', '/os-aggregates/1', pos=-1) - def test_aggregate_set_metadata_by_id(self): - self.run_command('aggregate-set-metadata 1 foo=bar delete_key') - body = {"set_metadata": {"metadata": {"foo": "bar", - "delete_key": None}}} - self.assert_called('POST', '/os-aggregates/1/action', body, pos=-2) - self.assert_called('GET', '/os-aggregates/1', pos=-1) + def test_aggregate_set_metadata_add_by_id(self): + self.run_command('aggregate-set-metadata 3 foo=bar') + body = {"set_metadata": {"metadata": {"foo": "bar"}}} + self.assert_called('POST', '/os-aggregates/3/action', body, pos=-2) + self.assert_called('GET', '/os-aggregates/3', pos=-1) + + def test_aggregate_set_metadata_add_duplicate_by_id(self): + cmd = 'aggregate-set-metadata 3 test=dup' + self.assertRaises(exceptions.CommandError, self.run_command, cmd) + + def test_aggregate_set_metadata_delete_by_id(self): + self.run_command('aggregate-set-metadata 3 none_key') + body = {"set_metadata": {"metadata": {"none_key": None}}} + self.assert_called('POST', '/os-aggregates/3/action', body, pos=-2) + self.assert_called('GET', '/os-aggregates/3', pos=-1) + + def test_aggregate_set_metadata_delete_missing_by_id(self): + cmd = 'aggregate-set-metadata 3 delete_key2' + self.assertRaises(exceptions.CommandError, self.run_command, cmd) def test_aggregate_set_metadata_by_name(self): - self.run_command('aggregate-set-metadata test foo=bar delete_key') - body = {"set_metadata": {"metadata": {"foo": "bar", - "delete_key": None}}} + self.run_command('aggregate-set-metadata test foo=bar') + body = {"set_metadata": {"metadata": {"foo": "bar"}}} self.assert_called('POST', '/os-aggregates/1/action', body, pos=-2) self.assert_called('GET', '/os-aggregates/1', pos=-1) diff --git a/novaclient/tests/v3/test_shell.py b/novaclient/tests/v3/test_shell.py index fbed7e11a..2d71fc6f9 100644 --- a/novaclient/tests/v3/test_shell.py +++ b/novaclient/tests/v3/test_shell.py @@ -127,17 +127,29 @@ class ShellTest(utils.TestCase): self.assert_called('PUT', '/os-aggregates/1', body, pos=-2) self.assert_called('GET', '/os-aggregates/1', pos=-1) - def test_aggregate_set_metadata_by_id(self): - self.run_command('aggregate-set-metadata 1 foo=bar delete_key') - body = {"set_metadata": {"metadata": {"foo": "bar", - "delete_key": None}}} - self.assert_called('POST', '/os-aggregates/1/action', body, pos=-2) - self.assert_called('GET', '/os-aggregates/1', pos=-1) + def test_aggregate_set_metadata_add_by_id(self): + self.run_command('aggregate-set-metadata 3 foo=bar') + body = {"set_metadata": {"metadata": {"foo": "bar"}}} + self.assert_called('POST', '/os-aggregates/3/action', body, pos=-2) + self.assert_called('GET', '/os-aggregates/3', pos=-1) + + def test_aggregate_set_metadata_add_duplicate_by_id(self): + cmd = 'aggregate-set-metadata 3 test=dup' + self.assertRaises(exceptions.CommandError, self.run_command, cmd) + + def test_aggregate_set_metadata_delete_by_id(self): + self.run_command('aggregate-set-metadata 3 none_key') + body = {"set_metadata": {"metadata": {"none_key": None}}} + self.assert_called('POST', '/os-aggregates/3/action', body, pos=-2) + self.assert_called('GET', '/os-aggregates/3', pos=-1) + + def test_aggregate_set_metadata_delete_missing_by_id(self): + cmd = 'aggregate-set-metadata 3 delete_key2' + self.assertRaises(exceptions.CommandError, self.run_command, cmd) def test_aggregate_set_metadata_by_name(self): - self.run_command('aggregate-set-metadata test foo=bar delete_key') - body = {"set_metadata": {"metadata": {"foo": "bar", - "delete_key": None}}} + self.run_command('aggregate-set-metadata test foo=bar') + body = {"set_metadata": {"metadata": {"foo": "bar"}}} self.assert_called('POST', '/os-aggregates/1/action', body, pos=-2) self.assert_called('GET', '/os-aggregates/1', pos=-1) diff --git a/novaclient/v1_1/shell.py b/novaclient/v1_1/shell.py index 3429164b5..a7e2c5ab8 100644 --- a/novaclient/v1_1/shell.py +++ b/novaclient/v1_1/shell.py @@ -2777,6 +2777,14 @@ def do_aggregate_set_metadata(cs, args): """Update the metadata associated with the aggregate.""" aggregate = _find_aggregate(cs, args.aggregate) metadata = _extract_metadata(args) + currentmetadata = getattr(aggregate, 'metadata', {}) + if set(metadata.items()) & set(currentmetadata.items()): + raise exceptions.CommandError(_("metadata already exists")) + for key, value in metadata.items(): + if value is None and key not in currentmetadata: + raise exceptions.CommandError(_("metadata key %s does not exist" + " hence can not be deleted") + % key) aggregate = cs.aggregates.set_metadata(aggregate.id, metadata) print(_("Metadata has been successfully updated for aggregate %s.") % aggregate.id) diff --git a/novaclient/v3/shell.py b/novaclient/v3/shell.py index ea1feff86..0f4de0087 100644 --- a/novaclient/v3/shell.py +++ b/novaclient/v3/shell.py @@ -2290,6 +2290,14 @@ def do_aggregate_set_metadata(cs, args): """Update the metadata associated with the aggregate.""" aggregate = _find_aggregate(cs, args.aggregate) metadata = _extract_metadata(args) + currentmetadata = getattr(aggregate, 'metadata', {}) + if set(metadata.items()) & set(currentmetadata.items()): + raise exceptions.CommandError("metadata already exists") + for key, value in metadata.items(): + if value is None and key not in currentmetadata: + raise exceptions.CommandError("metadata key %s does not exist" + " hence can not be deleted" + % key) aggregate = cs.aggregates.set_metadata(aggregate.id, metadata) print("Metadata has been successfully updated for aggregate %s." % aggregate.id)