From a8adb476f76688106b2ac30ad5eca55e8a5efd3c Mon Sep 17 00:00:00 2001 From: Jamie Lennox Date: Tue, 17 Dec 2013 15:30:51 +1000 Subject: [PATCH] Remove debug specific handling I think debug handling was initially done this way for CLI handling where we wanted to make sure only the correct information was printed to the console. However as logging.basicConfig sets up a stream handler on the root logging object I can't see any purpose to the debug handling in the actual HTTPClient. Further than this it is completely wrong that a client library is messing with it's logging level, this should be handled by an application. The debug flag is maintained and deprecated in HTTPClient and removed from the session object. There has been no release since the addition of session so there is no problem with compatibility. Change-Id: Ib00f3d93d099ed1a9dd25f17121610a7289f0061 --- keystoneclient/httpclient.py | 17 ++--------- keystoneclient/session.py | 47 ++++++++++++------------------- keystoneclient/shell.py | 1 - keystoneclient/tests/test_http.py | 38 +++++++++++++++++-------- 4 files changed, 46 insertions(+), 57 deletions(-) diff --git a/keystoneclient/httpclient.py b/keystoneclient/httpclient.py index a0dcf24b1..b1d03e54c 100644 --- a/keystoneclient/httpclient.py +++ b/keystoneclient/httpclient.py @@ -24,8 +24,6 @@ OpenStack Client interface. Handles the REST calls and responses. import logging from six.moves.urllib import parse as urlparse -import requests - try: import keyring import pickle @@ -96,9 +94,7 @@ class HTTPClient(object): :param string cert: DEPRECATED: use session. (optional) :param boolean insecure: DEPRECATED: use session. (optional) :param string original_ip: DEPRECATED: use session. (optional) - :param boolean debug: Enables debug logging of all request and - responses to identity service. - default False (optional) + :param boolean debug: DEPRECATED: use logging configuration. (optional) :param dict auth_ref: To allow for consumers of the client to manage their own caching strategy, you may initialize a client with a previously captured auth_reference @@ -240,20 +236,11 @@ class HTTPClient(object): session = client_session.Session(verify=verify, cert=session_cert, original_ip=original_ip, - timeout=timeout, - debug=debug) + timeout=timeout) self.session = session self.domain = '' - - # logging setup self.debug_log = debug - if self.debug_log and not _logger.handlers: - ch = logging.StreamHandler() - _logger.setLevel(logging.DEBUG) - _logger.addHandler(ch) - if hasattr(requests, 'logging'): - requests.logging.getLogger(requests.__name__).addHandler(ch) # keyring setup if use_keyring and keyring is None: diff --git a/keystoneclient/session.py b/keystoneclient/session.py index eeedf9b92..3e234e4ee 100644 --- a/keystoneclient/session.py +++ b/keystoneclient/session.py @@ -34,7 +34,7 @@ class Session(object): user_agent = None def __init__(self, session=None, original_ip=None, verify=True, cert=None, - timeout=None, debug=False, user_agent=None): + timeout=None, user_agent=None): """Maintains client communication state and common functionality. As much as possible the parameters to this class reflect and are passed @@ -70,7 +70,6 @@ class Session(object): self.verify = verify self.cert = cert self.timeout = None - self.debug = debug if timeout is not None: self.timeout = float(timeout) @@ -79,8 +78,8 @@ class Session(object): if user_agent is not None: self.user_agent = user_agent - def request(self, url, method, json=None, original_ip=None, debug=None, - logger=None, user_agent=None, **kwargs): + def request(self, url, method, json=None, original_ip=None, + user_agent=None, **kwargs): """Send an HTTP request with the specified characteristics. Wrapper around `requests.Session.request` to handle tasks such as @@ -94,11 +93,9 @@ class Session(object): :param string original_ip: Mark this request as forwarded for this ip. (optional) :param dict headers: Headers to be included in the request. (optional) - :param bool debug: Enable debug logging. (Defaults to False) :param kwargs: any other parameter that can be passed to requests.Session.request (such as `headers`) or `json` that will be encoded as JSON and used as `data` argument - :param logging.Logger logger: A logger to output to. (optional) :param json: Some data to be represented as JSON. (optional) :param string user_agent: A user_agent to use for the request. If present will override one present in headers. @@ -133,31 +130,24 @@ class Session(object): headers['Content-Type'] = 'application/json' kwargs['data'] = jsonutils.dumps(json) - if not logger: - logger = _logger - - if debug is None: - debug = self.debug - kwargs.setdefault('verify', self.verify) - if debug: - string_parts = ['curl -i'] + string_parts = ['curl -i'] - if method: - string_parts.extend([' -X ', method]) + if method: + string_parts.extend([' -X ', method]) - string_parts.extend([' ', url]) + string_parts.extend([' ', url]) - if headers: - for header in six.iteritems(headers): - string_parts.append(' -H "%s: %s"' % header) + if headers: + for header in six.iteritems(headers): + string_parts.append(' -H "%s: %s"' % header) - logger.debug('REQ: %s', ''.join(string_parts)) + _logger.debug('REQ: %s', ''.join(string_parts)) - data = kwargs.get('data') - if data: - logger.debug('REQ BODY: %s', data) + data = kwargs.get('data') + if data: + _logger.debug('REQ BODY: %s', data) try: resp = self.session.request(method, url, **kwargs) @@ -171,13 +161,12 @@ class Session(object): msg = 'Unable to establish connection to %s' % url raise exceptions.ConnectionError(msg) - if debug: - logger.debug('RESP: [%s] %s\nRESP BODY: %s\n', - resp.status_code, resp.headers, resp.text) + _logger.debug('RESP: [%s] %s\nRESP BODY: %s\n', + resp.status_code, resp.headers, resp.text) if resp.status_code >= 400: - logger.debug('Request returned failure status: %s', - resp.status_code) + _logger.debug('Request returned failure status: %s', + resp.status_code) raise exceptions.from_response(resp, method, url) return resp diff --git a/keystoneclient/shell.py b/keystoneclient/shell.py index 62b7c68b2..4892248fd 100644 --- a/keystoneclient/shell.py +++ b/keystoneclient/shell.py @@ -395,7 +395,6 @@ class OpenStackIdentityShell(object): key=args.os_key, cert=args.os_cert, insecure=args.insecure, - debug=args.debug, timeout=args.timeout) else: self.auth_check(args) diff --git a/keystoneclient/tests/test_http.py b/keystoneclient/tests/test_http.py index 6d5e09bed..85bdc68a2 100644 --- a/keystoneclient/tests/test_http.py +++ b/keystoneclient/tests/test_http.py @@ -15,11 +15,13 @@ # under the License. import httpretty +import logging import six from testtools import matchers from keystoneclient import exceptions from keystoneclient import httpclient +from keystoneclient import session from keystoneclient.tests import utils RESPONSE_BODY = '{"hi": "there"}' @@ -151,7 +153,17 @@ class BasicRequestTests(utils.TestCase): def setUp(self): super(BasicRequestTests, self).setUp() - self.logger = FakeLog() + self.logger_message = six.moves.cStringIO() + handler = logging.StreamHandler(self.logger_message) + handler.setLevel(logging.DEBUG) + + self.logger = logging.getLogger(session.__name__) + level = self.logger.getEffectiveLevel() + self.logger.setLevel(logging.DEBUG) + self.logger.addHandler(handler) + + self.addCleanup(self.logger.removeHandler, handler) + self.addCleanup(self.logger.setLevel, level) def request(self, method='GET', response='Test Response', status=200, url=None, **kwargs): @@ -160,8 +172,7 @@ class BasicRequestTests(utils.TestCase): httpretty.register_uri(method, url, body=response, status=status) - return httpclient.request(url, method, debug=True, - logger=self.logger, **kwargs) + return httpclient.request(url, method, **kwargs) @httpretty.activate def test_basic_params(self): @@ -173,13 +184,15 @@ class BasicRequestTests(utils.TestCase): self.assertEqual(httpretty.last_request().method, method) - self.assertThat(self.logger.debug_log, matchers.Contains('curl')) - self.assertThat(self.logger.debug_log, matchers.Contains('-X %s' % - method)) - self.assertThat(self.logger.debug_log, matchers.Contains(self.url)) + logger_message = self.logger_message.getvalue() - self.assertThat(self.logger.debug_log, matchers.Contains(str(status))) - self.assertThat(self.logger.debug_log, matchers.Contains(response)) + self.assertThat(logger_message, matchers.Contains('curl')) + self.assertThat(logger_message, matchers.Contains('-X %s' % + method)) + self.assertThat(logger_message, matchers.Contains(self.url)) + + self.assertThat(logger_message, matchers.Contains(str(status))) + self.assertThat(logger_message, matchers.Contains(response)) @httpretty.activate def test_headers(self): @@ -191,12 +204,13 @@ class BasicRequestTests(utils.TestCase): self.assertRequestHeaderEqual(k, v) for header in six.iteritems(headers): - self.assertThat(self.logger.debug_log, + self.assertThat(self.logger_message.getvalue(), matchers.Contains('-H "%s: %s"' % header)) @httpretty.activate def test_body(self): data = "BODY DATA" self.request(response=data) - self.assertThat(self.logger.debug_log, matchers.Contains('BODY:')) - self.assertThat(self.logger.debug_log, matchers.Contains(data)) + logger_message = self.logger_message.getvalue() + self.assertThat(logger_message, matchers.Contains('BODY:')) + self.assertThat(logger_message, matchers.Contains(data))