From 68df0e30c738950ded9865a0f9df58bef14cdd39 Mon Sep 17 00:00:00 2001 From: Ian Cordasco Date: Wed, 26 Oct 2016 12:30:24 -0500 Subject: [PATCH] Enable less code in cratonclient resources Previously, resource managers were overriding __init__, list, and create to include extra parameters, e.g., region_id. This slightly alters the base CRUDClient to accept extra request keyword arguments to merge into each request's arguments. Change-Id: Ia850417c7a94eea95d35c8a93141c70945d1a800 --- cratonclient/crud.py | 28 ++++++++++++++----- cratonclient/tests/unit/test_cells_shell.py | 24 ++++++++--------- cratonclient/tests/unit/test_crud.py | 30 ++++++++++++++++++--- cratonclient/tests/unit/test_hosts_shell.py | 24 ++++++++--------- cratonclient/tests/unit/test_inventory.py | 12 +++++++-- cratonclient/v1/cells.py | 16 ----------- cratonclient/v1/hosts.py | 16 ----------- cratonclient/v1/inventory.py | 4 +-- 8 files changed, 84 insertions(+), 70 deletions(-) diff --git a/cratonclient/crud.py b/cratonclient/crud.py index 9dc2c73..ae7cacd 100644 --- a/cratonclient/crud.py +++ b/cratonclient/crud.py @@ -24,10 +24,11 @@ class CRUDClient(object): base_path = None resource_class = None - def __init__(self, session, url): + def __init__(self, session, url, **extra_request_kwargs): """Initialize our Client with a session and base url.""" self.session = session self.url = url.rstrip('/') + self.extra_request_kwargs = extra_request_kwargs def build_url(self, path_arguments=None): """Build a complete URL from the url, base_path, and arguments. @@ -76,21 +77,34 @@ class CRUDClient(object): return url - def create(self, **kwargs): + def merge_request_arguments(self, request_kwargs, skip_merge): + """Merge the extra request arguments into the per-request args.""" + if skip_merge: + return + + keys = set(self.extra_request_kwargs.keys()) + missing_keys = keys.difference(request_kwargs.keys()) + for key in missing_keys: + request_kwargs[key] = self.extra_request_kwargs[key] + + def create(self, skip_merge=False, **kwargs): """Create a new item based on the keyword arguments provided.""" + self.merge_request_arguments(kwargs, skip_merge) url = self.build_url(path_arguments=kwargs) response = self.session.post(url, json=kwargs) return self.resource_class(self, response.json(), loaded=True) - def get(self, item_id=None, **kwargs): + def get(self, item_id=None, skip_merge=True, **kwargs): """Retrieve the item based on the keyword arguments provided.""" + self.merge_request_arguments(kwargs, skip_merge) kwargs.setdefault(self.key + '_id', item_id) url = self.build_url(path_arguments=kwargs) response = self.session.get(url) return self.resource_class(self, response.json(), loaded=True) - def list(self, **kwargs): + def list(self, skip_merge=False, **kwargs): """List the items from this endpoint.""" + self.merge_request_arguments(kwargs, skip_merge) url = self.build_url(path_arguments=kwargs) response = self.session.get(url, params=kwargs) return [ @@ -98,15 +112,17 @@ class CRUDClient(object): for item in response.json() ] - def update(self, item_id=None, **kwargs): + def update(self, item_id=None, skip_merge=True, **kwargs): """Update the item based on the keyword arguments provided.""" + self.merge_request_arguments(kwargs, skip_merge) kwargs.setdefault(self.key + '_id', item_id) url = self.build_url(path_arguments=kwargs) response = self.session.put(url, json=kwargs) return self.resource_class(self, response.json(), loaded=True) - def delete(self, item_id=None, **kwargs): + def delete(self, item_id=None, skip_merge=True, **kwargs): """Delete the item based on the keyword arguments provided.""" + self.merge_request_arguments(kwargs, skip_merge) kwargs.setdefault(self.key + '_id', item_id) url = self.build_url(path_arguments=kwargs) response = self.session.delete(url, params=kwargs) diff --git a/cratonclient/tests/unit/test_cells_shell.py b/cratonclient/tests/unit/test_cells_shell.py index 5a5f80a..732ee93 100644 --- a/cratonclient/tests/unit/test_cells_shell.py +++ b/cratonclient/tests/unit/test_cells_shell.py @@ -157,8 +157,8 @@ class TestCellsShell(base.ShellTestCase): client = mock.Mock() inventory = mock.Mock() inventory.cells = cells.CellManager(mock.ANY, - mock.ANY, - 'http://127.0.0.1/') + 'http://127.0.0.1/', + region_id=mock.ANY) client.inventory = mock.Mock(name='inventory') client.inventory.return_value = inventory cells_shell.do_cell_create(client, self.cell_valid_fields) @@ -170,8 +170,8 @@ class TestCellsShell(base.ShellTestCase): client = mock.Mock() inventory = mock.Mock() inventory.cells = cells.CellManager(mock.ANY, - mock.ANY, - 'http://127.0.0.1/') + 'http://127.0.0.1/', + region_id=mock.ANY) client.inventory = mock.Mock(name='inventory') client.inventory.return_value = inventory cells_shell.do_cell_create(client, self.cell_invalid_field) @@ -195,8 +195,8 @@ class TestCellsShell(base.ShellTestCase): client = mock.Mock() inventory = mock.Mock() inventory.cells = cells.CellManager(mock.ANY, - mock.ANY, - 'http://127.0.0.1/') + 'http://127.0.0.1/', + region_id=mock.ANY) client.inventory = mock.Mock(name='inventory') client.inventory.return_value = inventory valid_input = Namespace(region=1, @@ -212,8 +212,8 @@ class TestCellsShell(base.ShellTestCase): client = mock.Mock() inventory = mock.Mock() inventory.cells = cells.CellManager(mock.ANY, - mock.ANY, - 'http://127.0.0.1/') + 'http://127.0.0.1/', + region_id=mock.ANY) client.inventory = mock.Mock(name='inventory') client.inventory.return_value = inventory invalid_input = Namespace(region=1, @@ -243,8 +243,8 @@ class TestCellsShell(base.ShellTestCase): client = mock.Mock() inventory = mock.Mock() inventory.cells = cells.CellManager(mock.ANY, - mock.ANY, - 'http://127.0.0.1/') + 'http://127.0.0.1/', + region_id=mock.ANY) client.inventory = mock.Mock(name='inventory') client.inventory.return_value = inventory test_args = Namespace(id=1, region=1) @@ -268,8 +268,8 @@ class TestCellsShell(base.ShellTestCase): client = mock.Mock() inventory = mock.Mock() inventory.cells = cells.CellManager(mock.ANY, - mock.ANY, - 'http://127.0.0.1/') + 'http://127.0.0.1/', + region_id=mock.ANY) client.inventory = mock.Mock(name='inventory') client.inventory.return_value = inventory test_args = Namespace(id=1, region=1) diff --git a/cratonclient/tests/unit/test_crud.py b/cratonclient/tests/unit/test_crud.py index 24311b0..ec9c31b 100644 --- a/cratonclient/tests/unit/test_crud.py +++ b/cratonclient/tests/unit/test_crud.py @@ -30,10 +30,16 @@ class TestCRUDClient(base.TestCase): super(TestCRUDClient, self).setUp() self.session = mock.Mock() self.resource_spec = mock.Mock(spec=crud.Resource) - self.client = crud.CRUDClient(self.session, 'http://example.com/v1/') - self.client.base_path = '/test' - self.client.key = 'test_key' - self.client.resource_class = self.resource_spec + self.client = self.create_client() + + def create_client(self, **kwargs): + """Create and configure a basic CRUDClient.""" + client = crud.CRUDClient(self.session, 'http://example.com/v1/', + **kwargs) + client.base_path = '/test' + client.key = 'test_key' + client.resource_class = self.resource_spec + return client def test_strips_trailing_forward_slash_from_url(self): """Verify the client strips the trailing / in a URL.""" @@ -73,6 +79,22 @@ class TestCRUDClient(base.TestCase): }), ) + def test_merge_request_arguments(self): + """Verify we include extra request arguments.""" + client = self.create_client(extra_id=4321) + request_args = {} + + client.merge_request_arguments(request_args, skip_merge=False) + self.assertEqual({'extra_id': 4321}, request_args) + + def test_merge_request_arguments_skips_merging(self): + """Verify we include extra request arguments.""" + client = self.create_client(extra_id=4321) + request_args = {} + + client.merge_request_arguments(request_args, skip_merge=True) + self.assertEqual({}, request_args) + def test_create_generates_a_post_request(self): """Verify that using our create method will POST to our service.""" response = self.session.post.return_value = mock.Mock() diff --git a/cratonclient/tests/unit/test_hosts_shell.py b/cratonclient/tests/unit/test_hosts_shell.py index 1e315d1..a91dcc0 100644 --- a/cratonclient/tests/unit/test_hosts_shell.py +++ b/cratonclient/tests/unit/test_hosts_shell.py @@ -169,8 +169,8 @@ class TestHostsShell(base.ShellTestCase): client = mock.Mock() inventory = mock.Mock() inventory.hosts = hosts.HostManager(mock.ANY, - mock.ANY, - 'http://127.0.0.1/') + 'http://127.0.0.1/', + region_id=mock.ANY) client.inventory = mock.Mock(name='inventory') client.inventory.return_value = inventory hosts_shell.do_host_create(client, self.host_valid_fields) @@ -182,8 +182,8 @@ class TestHostsShell(base.ShellTestCase): client = mock.Mock() inventory = mock.Mock() inventory.hosts = hosts.HostManager(mock.ANY, - mock.ANY, - 'http://127.0.0.1/') + 'http://127.0.0.1/', + region_id=mock.ANY) client.inventory = mock.Mock(name='inventory') client.inventory.return_value = inventory hosts_shell.do_host_create(client, self.host_invalid_field) @@ -207,8 +207,8 @@ class TestHostsShell(base.ShellTestCase): client = mock.Mock() inventory = mock.Mock() inventory.hosts = hosts.HostManager(mock.ANY, - mock.ANY, - 'http://127.0.0.1/') + 'http://127.0.0.1/', + region_id=mock.ANY) client.inventory = mock.Mock(name='inventory') client.inventory.return_value = inventory valid_input = Namespace(region=1, @@ -224,8 +224,8 @@ class TestHostsShell(base.ShellTestCase): client = mock.Mock() inventory = mock.Mock() inventory.hosts = hosts.HostManager(mock.ANY, - mock.ANY, - 'http://127.0.0.1/') + 'http://127.0.0.1/', + region_id=mock.ANY) client.inventory = mock.Mock(name='inventory') client.inventory.return_value = inventory invalid_input = Namespace(region=1, @@ -255,8 +255,8 @@ class TestHostsShell(base.ShellTestCase): client = mock.Mock() inventory = mock.Mock() inventory.hosts = hosts.HostManager(mock.ANY, - mock.ANY, - 'http://127.0.0.1/') + 'http://127.0.0.1/', + region_id=mock.ANY) client.inventory = mock.Mock(name='inventory') client.inventory.return_value = inventory test_args = Namespace(id=1, region=1) @@ -280,8 +280,8 @@ class TestHostsShell(base.ShellTestCase): client = mock.Mock() inventory = mock.Mock() inventory.hosts = hosts.HostManager(mock.ANY, - mock.ANY, - 'http://127.0.0.1/') + 'http://127.0.0.1/', + region_id=mock.ANY) client.inventory = mock.Mock(name='inventory') client.inventory.return_value = inventory test_args = Namespace(id=1, region=1) diff --git a/cratonclient/tests/unit/test_inventory.py b/cratonclient/tests/unit/test_inventory.py index b8dcebe..c6fa6c7 100644 --- a/cratonclient/tests/unit/test_inventory.py +++ b/cratonclient/tests/unit/test_inventory.py @@ -28,7 +28,11 @@ class TestInventory(base.TestCase): url = 'https://10.1.1.0:8080/' region_id = 1 inventory.Inventory(session, url, region_id) - mock_hostmanager.assert_called_once_with(region_id, session, url) + mock_hostmanager.assert_called_once_with( + session, + url, + region_id=region_id, + ) @mock.patch('cratonclient.v1.cells.CellManager') def test_inventory_creates_cell_manager(self, cell_manager): @@ -37,4 +41,8 @@ class TestInventory(base.TestCase): url = 'https://10.1.1.0:8080/' region_id = 1 inventory.Inventory(session, url, region_id) - cell_manager.assert_called_once_with(region_id, session, url) + cell_manager.assert_called_once_with( + session, + url, + region_id=region_id, + ) diff --git a/cratonclient/v1/cells.py b/cratonclient/v1/cells.py index a62dc39..173420f 100644 --- a/cratonclient/v1/cells.py +++ b/cratonclient/v1/cells.py @@ -27,22 +27,6 @@ class CellManager(crud.CRUDClient): key = 'cell' base_path = '/cells' resource_class = Cell - region_id = 0 - - def __init__(self, region_id, session, url): - """Initialize our CellManager object with region, session, and url.""" - super(CellManager, self).__init__(session, url) - self.region_id = region_id - - def list(self, **kwargs): - """Retrieve the cells in a specific region.""" - kwargs['region_id'] = self.region_id - return super(CellManager, self).list(**kwargs) - - def create(self, **kwargs): - """Create a cell in a specific region.""" - kwargs['region_id'] = self.region_id - return super(CellManager, self).create(**kwargs) CELL_FIELDS = { diff --git a/cratonclient/v1/hosts.py b/cratonclient/v1/hosts.py index 07f26d2..cbf0172 100644 --- a/cratonclient/v1/hosts.py +++ b/cratonclient/v1/hosts.py @@ -27,22 +27,6 @@ class HostManager(crud.CRUDClient): key = 'host' base_path = '/hosts' resource_class = Host - region_id = 0 - - def __init__(self, region_id, session, url): - """Initialize our HostManager object with region, session and url.""" - super(HostManager, self).__init__(session, url) - self.region_id = region_id - - def list(self, **kwargs): - """Retrieve the hosts in a specific region.""" - kwargs['region_id'] = self.region_id - return super(HostManager, self).list(**kwargs) - - def create(self, **kwargs): - """Create a host in a specific region.""" - kwargs['region_id'] = self.region_id - return super(HostManager, self).create(**kwargs) HOST_FIELDS = { diff --git a/cratonclient/v1/inventory.py b/cratonclient/v1/inventory.py index d373cf3..72dc5be 100644 --- a/cratonclient/v1/inventory.py +++ b/cratonclient/v1/inventory.py @@ -31,6 +31,6 @@ class Inventory(object): 'https://10.1.1.0:8080/'. """ # TODO(cmspence): self.region = self.regions.get(region=region_id) - self.hosts = hosts.HostManager(region_id, session, url) - self.cells = cells.CellManager(region_id, session, url) + self.hosts = hosts.HostManager(session, url, region_id=region_id) + self.cells = cells.CellManager(session, url, region_id=region_id) # TODO(cmspence): self.users, self.projects, self.workflows