From 0d839308cf6052fb52cae7165195c2fb658f1ff6 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Fri, 27 Mar 2015 15:26:09 +0100 Subject: [PATCH] Proper errors handling for client * Introduce a specific exception class (subclasses requests.HTTPError to be backward compatible) * Fetch error message from response body * i18nize existing errors Change-Id: I71922895c3177720789c6933da79b8882bfa7127 Closes-Bug: #1428680 --- README.rst | 4 ++++ ironic_discoverd/client.py | 33 +++++++++++++++++++++------- ironic_discoverd/test/test_client.py | 27 ++++++++++++++++++++--- 3 files changed, 53 insertions(+), 11 deletions(-) diff --git a/README.rst b/README.rst index 135b2722b..40c75c513 100644 --- a/README.rst +++ b/README.rst @@ -364,6 +364,10 @@ See `1.1.0 release tracking page`_ for details. * Proper CLI tool implemented as a plugin for OpenStackClient_. + Also client now properly sets error message from the server in its exception. + This might be a breaking change, if you relied on exception message + previously. + * The default value for ``overwrite_existing`` configuration option was flipped, matching the default behavior for Ironic inspection. diff --git a/ironic_discoverd/client.py b/ironic_discoverd/client.py index 1f7b90d5a..65bc733c7 100644 --- a/ironic_discoverd/client.py +++ b/ironic_discoverd/client.py @@ -19,8 +19,11 @@ import json import requests import six +from ironic_discoverd.common.i18n import _ + _DEFAULT_URL = 'http://127.0.0.1:5050/v1' +_ERROR_ENCODING = 'utf-8' def _prepare(base_url, auth_token): @@ -31,6 +34,20 @@ def _prepare(base_url, auth_token): return base_url, headers +class ClientError(requests.HTTPError): + """Error returned from a server.""" + def __init__(self, response): + # discoverd returns error message in body + msg = response.content.decode(_ERROR_ENCODING) + super(ClientError, self).__init__(msg, response=response) + + @classmethod + def raise_if_needed(cls, response): + """Raise exception if response contains error.""" + if response.status_code >= 400: + raise cls(response) + + def introspect(uuid, base_url=None, auth_token=None, new_ipmi_password=None, new_ipmi_username=None): """Start introspection for a node. @@ -46,16 +63,16 @@ def introspect(uuid, base_url=None, auth_token=None, driver_info. """ if not isinstance(uuid, six.string_types): - raise TypeError("Expected string for uuid argument, got %r" % uuid) + raise TypeError(_("Expected string for uuid argument, got %r") % uuid) if new_ipmi_username and not new_ipmi_password: - raise ValueError("Setting IPMI user name requires a new password") + raise ValueError(_("Setting IPMI user name requires a new password")) base_url, headers = _prepare(base_url, auth_token) params = {'new_ipmi_username': new_ipmi_username, 'new_ipmi_password': new_ipmi_password} res = requests.post("%s/introspection/%s" % (base_url, uuid), headers=headers, params=params) - res.raise_for_status() + ClientError.raise_if_needed(res) def get_status(uuid, base_url=None, auth_token=None): @@ -69,12 +86,12 @@ def get_status(uuid, base_url=None, auth_token=None): :raises: *requests* library HTTP errors. """ if not isinstance(uuid, six.string_types): - raise TypeError("Expected string for uuid argument, got %r" % uuid) + raise TypeError(_("Expected string for uuid argument, got %r") % uuid) base_url, headers = _prepare(base_url, auth_token) res = requests.get("%s/introspection/%s" % (base_url, uuid), headers=headers) - res.raise_for_status() + ClientError.raise_if_needed(res) return res.json() @@ -84,14 +101,14 @@ def discover(uuids, base_url=None, auth_token=None): DEPRECATED. Use introspect instead. """ if not all(isinstance(s, six.string_types) for s in uuids): - raise TypeError("Expected list of strings for uuids argument, got %s" % - uuids) + raise TypeError(_("Expected list of strings for uuids argument, " + "got %s") % uuids) base_url, headers = _prepare(base_url, auth_token) headers['Content-Type'] = 'application/json' res = requests.post(base_url + "/discover", data=json.dumps(uuids), headers=headers) - res.raise_for_status() + ClientError.raise_if_needed(res) if __name__ == '__main__': # pragma: no cover diff --git a/ironic_discoverd/test/test_client.py b/ironic_discoverd/test/test_client.py index b047ff7e5..3d21ead40 100644 --- a/ironic_discoverd/test/test_client.py +++ b/ironic_discoverd/test/test_client.py @@ -19,7 +19,8 @@ from oslo_utils import uuidutils from ironic_discoverd import client -@mock.patch.object(client.requests, 'post', autospec=True) +@mock.patch.object(client.requests, 'post', autospec=True, + **{'return_value.status_code': 200}) class TestIntrospect(unittest.TestCase): def setUp(self): super(TestIntrospect, self).setUp() @@ -74,8 +75,15 @@ class TestIntrospect(unittest.TestCase): params={'new_ipmi_username': None, 'new_ipmi_password': None} ) + def test_failed(self, mock_post): + mock_post.return_value.status_code = 404 + mock_post.return_value.content = b"boom" + self.assertRaisesRegexp(client.ClientError, "boom", + client.introspect, self.uuid) -@mock.patch.object(client.requests, 'post', autospec=True) + +@mock.patch.object(client.requests, 'post', autospec=True, + **{'return_value.status_code': 200}) class TestDiscover(unittest.TestCase): def setUp(self): super(TestDiscover, self).setUp() @@ -97,8 +105,15 @@ class TestDiscover(unittest.TestCase): self.assertRaises(TypeError, client.discover, 42) self.assertRaises(TypeError, client.discover, [42]) + def test_failed(self, mock_post): + mock_post.return_value.status_code = 404 + mock_post.return_value.content = b"boom" + self.assertRaisesRegexp(client.ClientError, "boom", + client.discover, [self.uuid]) -@mock.patch.object(client.requests, 'get', autospec=True) + +@mock.patch.object(client.requests, 'get', autospec=True, + **{'return_value.status_code': 200}) class TestGetStatus(unittest.TestCase): def setUp(self): super(TestGetStatus, self).setUp() @@ -116,3 +131,9 @@ class TestGetStatus(unittest.TestCase): def test_invalid_input(self, _): self.assertRaises(TypeError, client.get_status, 42) + + def test_failed(self, mock_post): + mock_post.return_value.status_code = 404 + mock_post.return_value.content = b"boom" + self.assertRaisesRegexp(client.ClientError, "boom", + client.get_status, self.uuid)