From 5cef3f726e00ab2b22e3eca2b1050a431547fb85 Mon Sep 17 00:00:00 2001 From: Takashi NATSUME Date: Thu, 7 Jul 2016 22:19:33 +0900 Subject: [PATCH] Add a hacking rule for string interpolation at logging String interpolation should be delayed to be handled by the logging code, rather than being done at the point of the logging call. So add a hacking rule for it. See the oslo i18n guideline. * http://docs.openstack.org/developer/oslo.i18n/guidelines.html Change-Id: I91e8d59d508c594256d5f74514e62f8f928d1df5 Closes-Bug: #1596829 --- HACKING.rst | 1 + neutron/agent/l3/agent.py | 2 +- neutron/agent/linux/pd.py | 2 +- neutron/api/v2/resource_helper.py | 4 +-- neutron/db/dns_db.py | 18 +++++----- neutron/db/quota/driver.py | 2 +- neutron/hacking/checks.py | 26 ++++++++++++++ .../agent/ovs_dvr_neutron_agent.py | 4 +-- .../plugins/ml2/extensions/dns_integration.py | 18 +++++----- neutron/tests/unit/hacking/test_checks.py | 35 +++++++++++++++++++ 10 files changed, 87 insertions(+), 25 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index bc17b7f3a13..347c0d4a590 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -28,6 +28,7 @@ Neutron Specific Commandments - [N334] Use unittest2 uniformly across Neutron. - [N340] Check usage of .i18n (and neutron.i18n) - [N341] Check usage of _ from python builtins +- [N342] String interpolation should be delayed at logging calls. Creating Unit Tests ------------------- diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py index 912164746c8..95c8a97cdf3 100644 --- a/neutron/agent/l3/agent.py +++ b/neutron/agent/l3/agent.py @@ -219,7 +219,7 @@ class L3NATAgent(ha.AgentMixin, 'to retrieve service plugins enabled. ' 'Check connectivity to neutron server. ' 'Retrying... ' - 'Detailed message: %(msg)s.') % {'msg': e}) + 'Detailed message: %(msg)s.'), {'msg': e}) continue break diff --git a/neutron/agent/linux/pd.py b/neutron/agent/linux/pd.py index 85cfead37ed..c19853cf7b6 100644 --- a/neutron/agent/linux/pd.py +++ b/neutron/agent/linux/pd.py @@ -230,7 +230,7 @@ class PrefixDelegation(object): def _lla_available(self, gw_ifname, ns_name, lla_with_mask): llas = self._get_llas(gw_ifname, ns_name) if self._is_lla_active(lla_with_mask, llas): - LOG.debug("LLA %s is active now" % lla_with_mask) + LOG.debug("LLA %s is active now", lla_with_mask) self.pd_update_cb() return True diff --git a/neutron/api/v2/resource_helper.py b/neutron/api/v2/resource_helper.py index bbdc2a110e9..1a10c01489e 100644 --- a/neutron/api/v2/resource_helper.py +++ b/neutron/api/v2/resource_helper.py @@ -78,8 +78,8 @@ def build_resource_info(plural_mappings, resource_map, which_service, else: plugin = manager.NeutronManager.get_plugin() path_prefix = getattr(plugin, "path_prefix", "") - LOG.debug('Service %(service)s assigned prefix: %(prefix)s' - % {'service': which_service, 'prefix': path_prefix}) + LOG.debug('Service %(service)s assigned prefix: %(prefix)s', + {'service': which_service, 'prefix': path_prefix}) for collection_name in resource_map: resource_name = plural_mappings[collection_name] params = resource_map.get(collection_name, {}) diff --git a/neutron/db/dns_db.py b/neutron/db/dns_db.py index 5395273cebf..d3d7d8458a7 100644 --- a/neutron/db/dns_db.py +++ b/neutron/db/dns_db.py @@ -287,11 +287,11 @@ class DNSDbMixin(object): LOG.exception(_LE("Error deleting Floating IP data from external " "DNS service. Name: '%(name)s'. Domain: " "'%(domain)s'. IP addresses '%(ips)s'. DNS " - "service driver message '%(message)s'") - % {"name": dns_name, - "domain": dns_domain, - "message": e.msg, - "ips": ', '.join(records)}) + "service driver message '%(message)s'"), + {"name": dns_name, + "domain": dns_domain, + "message": e.msg, + "ips": ', '.join(records)}) def _get_requested_state_for_external_dns_service_create(self, context, floatingip_data, @@ -318,7 +318,7 @@ class DNSDbMixin(object): LOG.exception(_LE("Error publishing floating IP data in external " "DNS service. Name: '%(name)s'. Domain: " "'%(domain)s'. DNS service driver message " - "'%(message)s'") - % {"name": dns_name, - "domain": dns_domain, - "message": e.msg}) + "'%(message)s'"), + {"name": dns_name, + "domain": dns_domain, + "message": e.msg}) diff --git a/neutron/db/quota/driver.py b/neutron/db/quota/driver.py index 7b5464055a3..a063e4d8027 100644 --- a/neutron/db/quota/driver.py +++ b/neutron/db/quota/driver.py @@ -151,7 +151,7 @@ class DbQuotaDriver(object): return dict((k, v) for k, v in quotas.items()) def _handle_expired_reservations(self, context, tenant_id): - LOG.debug("Deleting expired reservations for tenant:%s" % tenant_id) + LOG.debug("Deleting expired reservations for tenant:%s", tenant_id) # Delete expired reservations (we don't want them to accrue # in the database) quota_api.remove_expired_reservations( diff --git a/neutron/hacking/checks.py b/neutron/hacking/checks.py index e7364f2bb6f..1ef4ebabd7a 100644 --- a/neutron/hacking/checks.py +++ b/neutron/hacking/checks.py @@ -64,6 +64,9 @@ def _regex_for_level(level, hint): } +log_string_interpolation = re.compile(r".*LOG\.(?:error|warn|warning|info" + r"|critical|exception|debug)" + r"\([^,]*%[^,]*[,)]") log_translation_hint = re.compile( '|'.join('(?:%s)' % _regex_for_level(level, hint) for level, hint in six.iteritems(_all_log_levels))) @@ -356,6 +359,28 @@ def check_unittest_imports(logical_line): yield (0, msg) +@flake8ext +def check_delayed_string_interpolation(logical_line, filename, noqa): + """N342 String interpolation should be delayed at logging calls. + + N342: LOG.debug('Example: %s' % 'bad') + Okay: LOG.debug('Example: %s', 'good') + """ + msg = ("N342 String interpolation should be delayed to be " + "handled by the logging code, rather than being done " + "at the point of the logging call. " + "Use ',' instead of '%'.") + + if noqa: + return + + if 'neutron/tests/' in filename: + return + + if log_string_interpolation.match(logical_line): + yield(0, msg) + + def factory(register): register(validate_log_translations) register(use_jsonutils) @@ -374,3 +399,4 @@ def factory(register): register(check_oslo_i18n_wrapper) register(check_builtins_gettext) register(check_unittest_imports) + register(check_delayed_string_interpolation) diff --git a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py index 4c5bd434846..4c98af57ff0 100644 --- a/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py +++ b/neutron/plugins/ml2/drivers/openvswitch/agent/ovs_dvr_neutron_agent.py @@ -570,8 +570,8 @@ class OVSDVRNeutronAgent(object): if local_vlan_map.network_type not in (constants.TUNNEL_NETWORK_TYPES + [p_const.TYPE_VLAN]): LOG.debug("DVR: Port %s is with network_type %s not supported" - " for dvr plumbing" % (port.vif_id, - local_vlan_map.network_type)) + " for dvr plumbing", port.vif_id, + local_vlan_map.network_type) return if (port.vif_id in self.local_ports and diff --git a/neutron/plugins/ml2/extensions/dns_integration.py b/neutron/plugins/ml2/extensions/dns_integration.py index 1a24448d5f1..e900cb3ac99 100644 --- a/neutron/plugins/ml2/extensions/dns_integration.py +++ b/neutron/plugins/ml2/extensions/dns_integration.py @@ -248,10 +248,10 @@ def _send_data_to_external_dns_service(context, dns_driver, dns_domain, except (dns.DNSDomainNotFound, dns.DuplicateRecordSet) as e: LOG.exception(_LE("Error publishing port data in external DNS " "service. Name: '%(name)s'. Domain: '%(domain)s'. " - "DNS service driver message '%(message)s'") - % {"name": dns_name, - "domain": dns_domain, - "message": e.msg}) + "DNS service driver message '%(message)s'"), + {"name": dns_name, + "domain": dns_domain, + "message": e.msg}) def _remove_data_from_external_dns_service(context, dns_driver, dns_domain, @@ -262,11 +262,11 @@ def _remove_data_from_external_dns_service(context, dns_driver, dns_domain, LOG.exception(_LE("Error deleting port data from external DNS " "service. Name: '%(name)s'. Domain: '%(domain)s'. " "IP addresses '%(ips)s'. DNS service driver message " - "'%(message)s'") - % {"name": dns_name, - "domain": dns_domain, - "message": e.msg, - "ips": ', '.join(records)}) + "'%(message)s'"), + {"name": dns_name, + "domain": dns_domain, + "message": e.msg, + "ips": ', '.join(records)}) def _update_port_in_external_dns_service(resource, event, trigger, **kwargs): diff --git a/neutron/tests/unit/hacking/test_checks.py b/neutron/tests/unit/hacking/test_checks.py index 28ad3bdb33f..dafb0a980c6 100644 --- a/neutron/tests/unit/hacking/test_checks.py +++ b/neutron/tests/unit/hacking/test_checks.py @@ -24,6 +24,9 @@ from neutron.hacking import checks from neutron.tests import base +CREATE_DUMMY_MATCH_OBJECT = re.compile('a') + + class HackingTestCase(base.BaseTestCase): def assertLinePasses(self, func, line): @@ -298,6 +301,38 @@ class HackingTestCase(base.BaseTestCase): self.assertLineFails(f, 'from unittest.TestSuite') self.assertLineFails(f, 'import unittest') + def test_check_delayed_string_interpolation(self): + dummy_noqa = CREATE_DUMMY_MATCH_OBJECT.search('a') + + # In 'logical_line', Contents of strings replaced with + # "xxx" of same length. + fail_code1 = 'LOG.error(_LE("xxxxxxxxxxxxxxx") % value)' + fail_code2 = "LOG.warning(msg % 'xxxxx')" + + self.assertEqual( + 1, len(list(checks.check_delayed_string_interpolation(fail_code1, + "neutron/common/rpc.py", None)))) + self.assertEqual( + 1, len(list(checks.check_delayed_string_interpolation(fail_code2, + "neutron/common/rpc.py", None)))) + + pass_code1 = 'LOG.error(_LE("xxxxxxxxxxxxxxxxxx"), value)' + pass_code2 = "LOG.warning(msg, 'xxxxx')" + self.assertEqual( + 0, len(list(checks.check_delayed_string_interpolation(pass_code1, + "neutron/common/rpc.py", None)))) + self.assertEqual( + 0, len(list(checks.check_delayed_string_interpolation(pass_code2, + "neutron/common/rpc.py", None)))) + # check a file in neutron/tests + self.assertEqual( + 0, len(list(checks.check_delayed_string_interpolation(fail_code1, + "neutron/tests/test_assert.py", + None)))) + # check code including 'noqa' + self.assertEqual( + 0, len(list(checks.check_delayed_string_interpolation(fail_code1, + "neutron/common/rpc.py", dummy_noqa)))) # The following is borrowed from hacking/tests/test_doctest.py. # Tests defined in docstring is easier to understand