From 1f7b0efdcca65d1940f0e982d481651de003710e Mon Sep 17 00:00:00 2001 From: Sean McGinnis Date: Thu, 19 Dec 2019 10:46:37 -0600 Subject: [PATCH] Enable flake8-logging-format extension The flake8-logging-format extension includes several checks for things we've had to try to catch in code reviews until now. This enables the extension and fixes the few cases where things had slipped through code review. G200: Logging statements should not include the exception in logged string is disabled since that triggers a lot more issues, some of which may be acceptable. That can be left as a follow up exercise if we want to clean those up and enable all checks. Change-Id: I1dedc0b31f78f518c2ab5dee5ed7abda1c1d9296 Signed-off-by: Sean McGinnis --- cinder/api/extensions.py | 2 +- cinder/api/openstack/wsgi.py | 6 ++---- cinder/privsep/hscli.py | 5 ++--- cinder/tests/hacking/checks.py | 7 ------- cinder/volume/drivers/linstordrv.py | 2 +- cinder/volume/drivers/stx/client.py | 4 ++-- cinder/volume/flows/api/create_volume.py | 2 +- cinder/volume/flows/api/manage_existing.py | 2 +- lower-constraints.txt | 1 + test-requirements.txt | 1 + tox.ini | 4 +++- 11 files changed, 15 insertions(+), 21 deletions(-) diff --git a/cinder/api/extensions.py b/cinder/api/extensions.py index 9ed73d8f7f0..09ab1dc7e87 100644 --- a/cinder/api/extensions.py +++ b/cinder/api/extensions.py @@ -281,7 +281,7 @@ def load_standard_extensions(ext_mgr, logger, path, package, ext_list=None): (package, relpkg, root, classname)) if ext_list is not None and classname not in ext_list: - logger.debug("Skipping extension: %s" % classpath) + logger.debug("Skipping extension: %s", classpath) continue try: diff --git a/cinder/api/openstack/wsgi.py b/cinder/api/openstack/wsgi.py index 59c4bc6fb19..447d9f1e1b7 100644 --- a/cinder/api/openstack/wsgi.py +++ b/cinder/api/openstack/wsgi.py @@ -600,13 +600,11 @@ class ResourceExceptionHandler(object): raise Fault(exception.ConvertedException( code=ex_value.code, explanation=six.text_type(ex_value))) elif isinstance(ex_value, TypeError): - exc_info = (ex_type, ex_value, ex_traceback) - LOG.error('Exception handling resource: %s', - ex_value, exc_info=exc_info) + LOG.exception('Exception handling resource:') raise Fault(webob.exc.HTTPBadRequest()) elif isinstance(ex_value, Fault): LOG.info("Fault thrown: %s", ex_value) - raise ex_value + raise elif isinstance(ex_value, webob.exc.HTTPException): LOG.info("HTTP exception thrown: %s", ex_value) raise Fault(ex_value) diff --git a/cinder/privsep/hscli.py b/cinder/privsep/hscli.py index 4d7f85d6610..fcbfd35be09 100644 --- a/cinder/privsep/hscli.py +++ b/cinder/privsep/hscli.py @@ -37,9 +37,8 @@ def hsexecute(cmdarg_json): (cmd_out, cmd_err) = putils.execute("hscli", cmdarg_json) except (putils.UnknownArgumentError, putils.ProcessExecutionError, OSError): - LOG.error("Exception in running the command for %s", - cmdarg_json, - exc_info=True) + LOG.exception("Exception in running the command for %s", + cmdarg_json) raise exception.UnableToExecuteHyperScaleCmd(command=cmdarg_json) return (cmd_out, cmd_err) diff --git a/cinder/tests/hacking/checks.py b/cinder/tests/hacking/checks.py index 5caa6574a56..2e590127773 100644 --- a/cinder/tests/hacking/checks.py +++ b/cinder/tests/hacking/checks.py @@ -416,12 +416,6 @@ def check_timeutils_strtime(logical_line): yield(0, msg) -def no_log_warn(logical_line): - msg = "C307: LOG.warn is deprecated, please use LOG.warning!" - if "LOG.warn(" in logical_line: - yield (0, msg) - - def dict_constructor_with_list_copy(logical_line): msg = ("N336: Must use a dict comprehension instead of a dict constructor " "with a sequence of key-value pairs.") @@ -470,7 +464,6 @@ def factory(register): register(check_unicode_usage) register(check_no_print_statements) register(check_no_log_audit) - register(no_log_warn) register(dict_constructor_with_list_copy) register(no_test_log) register(validate_assertTrue) diff --git a/cinder/volume/drivers/linstordrv.py b/cinder/volume/drivers/linstordrv.py index 9beb820456e..ff9e3eeed84 100644 --- a/cinder/volume/drivers/linstordrv.py +++ b/cinder/volume/drivers/linstordrv.py @@ -1036,7 +1036,7 @@ class LinstorIscsiDriver(LinstorBaseDriver): db=self.db, executor=self._execute) - LOG.info('START: LINSTOR DRBD driver {}'.format(self.helper_name)) + LOG.info('START: LINSTOR DRBD driver %s', self.helper_name) def get_volume_stats(self, refresh=False): data = self._get_volume_stats() diff --git a/cinder/volume/drivers/stx/client.py b/cinder/volume/drivers/stx/client.py index d8eca8a4f87..9b0f62ee8d2 100644 --- a/cinder/volume/drivers/stx/client.py +++ b/cinder/volume/drivers/stx/client.py @@ -392,8 +392,8 @@ class STXClient(object): lun = obj.findtext("PROPERTY[@name='lun']") iid = obj.findtext("PROPERTY[@name='identifier']") if iid in ids: - LOG.debug("volume '{}' is already mapped to {} at lun {}". - format(volume_name, iid, lun)) + LOG.debug("volume '%s' is already mapped to %s at lun %s", + volume_name, iid, lun) return int(lun) except Exception: LOG.exception("failed to look up mappings for volume '%s'", diff --git a/cinder/volume/flows/api/create_volume.py b/cinder/volume/flows/api/create_volume.py index b3a77b4b104..4e57eae1dc1 100644 --- a/cinder/volume/flows/api/create_volume.py +++ b/cinder/volume/flows/api/create_volume.py @@ -799,7 +799,7 @@ class VolumeCastTask(flow_utils.CinderTask): exc_info = False if all(flow_failures[-1].exc_info): exc_info = flow_failures[-1].exc_info - LOG.error('Unexpected build error:', exc_info=exc_info) + LOG.error('Unexpected build error:', exc_info=exc_info) # noqa def get_flow(db_api, image_service_api, availability_zones, create_what, diff --git a/cinder/volume/flows/api/manage_existing.py b/cinder/volume/flows/api/manage_existing.py index 3c1e94cc7ab..b06f418c7f4 100644 --- a/cinder/volume/flows/api/manage_existing.py +++ b/cinder/volume/flows/api/manage_existing.py @@ -126,7 +126,7 @@ class ManageCastTask(flow_utils.CinderTask): exc_info = False if all(flow_failures[-1].exc_info): exc_info = flow_failures[-1].exc_info - LOG.error('Unexpected build error:', exc_info=exc_info) + LOG.error('Unexpected build error:', exc_info=exc_info) # noqa def get_flow(scheduler_rpcapi, db_api, create_what): diff --git a/lower-constraints.txt b/lower-constraints.txt index 35c705e3397..292615a4d3f 100644 --- a/lower-constraints.txt +++ b/lower-constraints.txt @@ -33,6 +33,7 @@ fasteners==0.14.1 fixtures==3.0.0 flake8==2.5.5 flake8-import-order==0.18.1 +flake8-logging-format==0.6.0 future==0.16.0 futurist==1.6.0 gitdb2==2.0.3 diff --git a/test-requirements.txt b/test-requirements.txt index d886ac6e389..3ba45e78116 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -5,6 +5,7 @@ # Install bounded pep8/pyflakes first, then let flake8 install hacking>=2.0.0 # Apache-2.0 flake8-import-order # LGPLv3 +flake8-logging-format>=0.6.0 # Apache-2.0 coverage!=4.4,>=4.0 # Apache-2.0 ddt>=1.2.1 # MIT diff --git a/tox.ini b/tox.ini index 5aa2a609409..9ebc3555bae 100644 --- a/tox.ini +++ b/tox.ini @@ -188,7 +188,9 @@ usedevelop = False # tools from getting in our way with regards to this. # H101 include name with TODO # reason: no real benefit -ignore = E251,E402,W503,W504,H101 +# G200 Logging statements should not include the exception +# reason: Many existing cases of this that may be legitimate +ignore = E251,E402,W503,W504,H101,G200 # H904 Delay string interpolations at logging calls. enable-extensions = H106,H203,H904 exclude = .git,.venv,.tox,dist,tools,doc/ext,*egg,build