From c5c2d67b7f683ea1f4e8acfb54930bfc2954b342 Mon Sep 17 00:00:00 2001 From: Anton Arefiev Date: Tue, 28 Apr 2015 13:12:48 +0300 Subject: [PATCH] Find resource refactoring The 'name' field for some resources is called something different, for example 'display_name' for volumes, 'name' for volumes type. So class 'resource' has attribute 'NAME_ATTR' wich contains attribute name for different resources. This change removes reduntant call find in resource manager, instead of it checks NAME_ATTR value. Related-Bug: #1449444 Change-Id: Ide334d7535c73cbdff72c30319eb539d8f5304d6 --- cinderclient/tests/unit/test_utils.py | 33 ++++++++++++++++++------ cinderclient/tests/unit/v1/fakes.py | 6 ++--- cinderclient/tests/unit/v1/test_shell.py | 2 +- cinderclient/utils.py | 21 +++++++-------- cinderclient/v1/volume_backups.py | 2 ++ cinderclient/v1/volume_snapshots.py | 2 ++ cinderclient/v1/volume_transfers.py | 2 ++ cinderclient/v1/volumes.py | 2 ++ cinderclient/v2/volume_backups.py | 2 ++ cinderclient/v2/volume_snapshots.py | 2 ++ cinderclient/v2/volume_transfers.py | 2 ++ 11 files changed, 52 insertions(+), 24 deletions(-) diff --git a/cinderclient/tests/unit/test_utils.py b/cinderclient/tests/unit/test_utils.py index 0228cf672..73a39b98f 100644 --- a/cinderclient/tests/unit/test_utils.py +++ b/cinderclient/tests/unit/test_utils.py @@ -26,6 +26,7 @@ UUID = '8e8ec658-c7b0-4243-bdf8-6f7f2952c0d0' class FakeResource(object): + NAME_ATTR = 'name' def __init__(self, _id, properties): self.id = _id @@ -33,10 +34,6 @@ class FakeResource(object): self.name = properties['name'] except KeyError: pass - try: - self.display_name = properties['display_name'] - except KeyError: - pass class FakeManager(base.ManagerWithFind): @@ -46,7 +43,6 @@ class FakeManager(base.ManagerWithFind): resources = [ FakeResource('1234', {'name': 'entity_one'}), FakeResource(UUID, {'name': 'entity_two'}), - FakeResource('4242', {'display_name': 'entity_three'}), FakeResource('5678', {'name': '9876'}) ] @@ -60,6 +56,26 @@ class FakeManager(base.ManagerWithFind): return self.resources +class FakeDisplayResource(object): + NAME_ATTR = 'display_name' + + def __init__(self, _id, properties): + self.id = _id + try: + self.display_name = properties['display_name'] + except KeyError: + pass + + +class FakeDisplayManager(FakeManager): + + resource_class = FakeDisplayResource + + resources = [ + FakeDisplayResource('4242', {'display_name': 'entity_three'}), + ] + + class FindResourceTestCase(test_utils.TestCase): def setUp(self): @@ -72,7 +88,7 @@ class FindResourceTestCase(test_utils.TestCase): utils.find_resource, self.manager, 'asdf') - self.assertEqual(3, self.manager.find.call_count) + self.assertEqual(2, self.manager.find.call_count) def test_find_by_integer_id(self): output = utils.find_resource(self.manager, 1234) @@ -91,8 +107,9 @@ class FindResourceTestCase(test_utils.TestCase): self.assertEqual(self.manager.get('1234'), output) def test_find_by_str_displayname(self): - output = utils.find_resource(self.manager, 'entity_three') - self.assertEqual(self.manager.get('4242'), output) + display_manager = FakeDisplayManager(None) + output = utils.find_resource(display_manager, 'entity_three') + self.assertEqual(display_manager.get('4242'), output) class CaptureStdout(object): diff --git a/cinderclient/tests/unit/v1/fakes.py b/cinderclient/tests/unit/v1/fakes.py index a5cb0f5d4..e391dc927 100644 --- a/cinderclient/tests/unit/v1/fakes.py +++ b/cinderclient/tests/unit/v1/fakes.py @@ -299,8 +299,8 @@ class FakeHTTPClient(base_client.HTTPClient): def get_volumes(self, **kw): return (200, {}, {"volumes": [ - {'id': 1234, 'name': 'sample-volume'}, - {'id': 5678, 'name': 'sample-volume2'} + {'id': 1234, 'display_name': 'sample-volume'}, + {'id': 5678, 'display_name': 'sample-volume2'} ]}) # TODO(jdg): This will need to change @@ -308,7 +308,7 @@ class FakeHTTPClient(base_client.HTTPClient): def get_volumes_detail(self, **kw): return (200, {}, {"volumes": [ {'id': kw.get('id', 1234), - 'name': 'sample-volume', + 'display_name': 'sample-volume', 'attachments': [{'server_id': 1234}]}, ]}) diff --git a/cinderclient/tests/unit/v1/test_shell.py b/cinderclient/tests/unit/v1/test_shell.py index 4eb749daf..44e7b09b9 100644 --- a/cinderclient/tests/unit/v1/test_shell.py +++ b/cinderclient/tests/unit/v1/test_shell.py @@ -115,7 +115,7 @@ class ShellTest(utils.TestCase): v = cs.volumes.list()[0] setattr(v, 'os-vol-tenant-attr:tenant_id', 'fake_tenant') setattr(v, '_info', {'attachments': [{'server_id': 1234}], - 'id': 1234, 'name': 'sample-volume', + 'id': 1234, 'display_name': 'sample-volume', 'os-vol-tenant-attr:tenant_id': 'fake_tenant'}) shell_v1._translate_volume_keys([v]) self.assertEqual(v.tenant_id, 'fake_tenant') diff --git a/cinderclient/utils.py b/cinderclient/utils.py index cb9125c68..896c6467b 100644 --- a/cinderclient/utils.py +++ b/cinderclient/utils.py @@ -178,24 +178,21 @@ def find_resource(manager, name_or_id): name_or_id = strutils.safe_decode(name_or_id) try: + try: + resource = getattr(manager, 'resource_class', None) + name_attr = resource.NAME_ATTR if resource else 'name' + return manager.find(**{name_attr: name_or_id}) + except exceptions.NotFound: + pass + + # finally try to find entity by human_id try: return manager.find(human_id=name_or_id) except exceptions.NotFound: - pass - - # finally try to find entity by name - try: - return manager.find(name=name_or_id) - except exceptions.NotFound: - pass - - # Volumes don't have name, but display_name - try: - return manager.find(display_name=name_or_id) - except (UnicodeDecodeError, exceptions.NotFound): msg = "No %s with a name or ID of '%s' exists." % \ (manager.resource_class.__name__.lower(), name_or_id) raise exceptions.CommandError(msg) + except exceptions.NoUniqueMatch: msg = ("Multiple %s matches found for '%s', use an ID to be more" " specific." % (manager.resource_class.__name__.lower(), diff --git a/cinderclient/v1/volume_backups.py b/cinderclient/v1/volume_backups.py index 6c492e9d9..4040d5d5f 100644 --- a/cinderclient/v1/volume_backups.py +++ b/cinderclient/v1/volume_backups.py @@ -22,6 +22,8 @@ from cinderclient import base class VolumeBackup(base.Resource): """A volume backup is a block level backup of a volume.""" + NAME_ATTR = "display_name" + def __repr__(self): return "" % self.id diff --git a/cinderclient/v1/volume_snapshots.py b/cinderclient/v1/volume_snapshots.py index 690fd0123..3b1e17c80 100644 --- a/cinderclient/v1/volume_snapshots.py +++ b/cinderclient/v1/volume_snapshots.py @@ -30,6 +30,8 @@ class Snapshot(base.Resource): """ A Snapshot is a point-in-time snapshot of an openstack volume. """ + NAME_ATTR = "display_name" + def __repr__(self): return "" % self.id diff --git a/cinderclient/v1/volume_transfers.py b/cinderclient/v1/volume_transfers.py index 23317d2cd..a562c1500 100644 --- a/cinderclient/v1/volume_transfers.py +++ b/cinderclient/v1/volume_transfers.py @@ -27,6 +27,8 @@ from cinderclient import base class VolumeTransfer(base.Resource): """Transfer a volume from one tenant to another""" + NAME_ATTR = "display_name" + def __repr__(self): return "" % self.id diff --git a/cinderclient/v1/volumes.py b/cinderclient/v1/volumes.py index 73600d646..3ef3181c2 100644 --- a/cinderclient/v1/volumes.py +++ b/cinderclient/v1/volumes.py @@ -27,6 +27,8 @@ from cinderclient import base class Volume(base.Resource): """A volume is an extra block level storage to the OpenStack instances.""" + NAME_ATTR = "display_name" + def __repr__(self): return "" % self.id diff --git a/cinderclient/v2/volume_backups.py b/cinderclient/v2/volume_backups.py index faee6b177..6a4fb9555 100644 --- a/cinderclient/v2/volume_backups.py +++ b/cinderclient/v2/volume_backups.py @@ -22,6 +22,8 @@ from cinderclient import base class VolumeBackup(base.Resource): """A volume backup is a block level backup of a volume.""" + NAME_ATTR = "display_name" + def __repr__(self): return "" % self.id diff --git a/cinderclient/v2/volume_snapshots.py b/cinderclient/v2/volume_snapshots.py index 51b611db6..90ea1cfcc 100644 --- a/cinderclient/v2/volume_snapshots.py +++ b/cinderclient/v2/volume_snapshots.py @@ -26,6 +26,8 @@ from cinderclient import base class Snapshot(base.Resource): """A Snapshot is a point-in-time snapshot of an openstack volume.""" + NAME_ATTR = "display_name" + def __repr__(self): return "" % self.id diff --git a/cinderclient/v2/volume_transfers.py b/cinderclient/v2/volume_transfers.py index 23317d2cd..a562c1500 100644 --- a/cinderclient/v2/volume_transfers.py +++ b/cinderclient/v2/volume_transfers.py @@ -27,6 +27,8 @@ from cinderclient import base class VolumeTransfer(base.Resource): """Transfer a volume from one tenant to another""" + NAME_ATTR = "display_name" + def __repr__(self): return "" % self.id