From 90ed02d68256253820343c6f7b4be78dfb3d9e37 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Cuong Date: Mon, 3 Jul 2017 04:13:46 -0400 Subject: [PATCH] Delete log translation functions and add hacking rule The i18n team has decided not to translate the logs because it seems like it not very useful; operators prefer to have them in English so that they can search for those strings on the internet. Since we have removed log translations completely, we should add hacking rule to prevent future mistakes. Change-Id: Ia7524308ef2675f8d41ac80b37dfc7e3787efd90 --- HACKING.rst | 4 +-- ec2api/api/__init__.py | 4 +-- ec2api/api/apirequest.py | 4 +-- ec2api/api/ec2utils.py | 2 +- ec2api/api/subnet.py | 4 +-- ec2api/api/volume.py | 2 +- ec2api/api/vpc.py | 4 +-- ec2api/api/vpn_connection.py | 15 +++++---- ec2api/api/vpn_gateway.py | 4 +-- ec2api/exception.py | 8 ++--- ec2api/hacking/__init__.py | 0 ec2api/hacking/checks.py | 52 +++++++++++++++++++++++++++++++ ec2api/metadata/api.py | 8 ++--- ec2api/tests/unit/test_hacking.py | 29 +++++++++++++++++ ec2api/utils.py | 3 +- ec2api/wsgi.py | 14 ++++----- tox.ini | 2 +- 17 files changed, 119 insertions(+), 40 deletions(-) create mode 100644 ec2api/hacking/__init__.py create mode 100644 ec2api/hacking/checks.py create mode 100644 ec2api/tests/unit/test_hacking.py diff --git a/HACKING.rst b/HACKING.rst index 146bc497..8dbfe564 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -12,10 +12,10 @@ General ------- - Do not use locals(). Example:: - LOG.debug(_("volume %(vol_name)s: creating size %(vol_size)sG") % + LOG.debug("volume %(vol_name)s: creating size %(vol_size)sG" % locals()) # BAD - LOG.debug(_("volume %(vol_name)s: creating size %(vol_size)sG") % + LOG.debug("volume %(vol_name)s: creating size %(vol_size)sG" % {'vol_name': vol_name, 'vol_size': vol_size}) # OKAY diff --git a/ec2api/api/__init__.py b/ec2api/api/__init__.py index d783b47b..385ecb65 100644 --- a/ec2api/api/__init__.py +++ b/ec2api/api/__init__.py @@ -75,7 +75,7 @@ class FaultWrapper(wsgi.Middleware): try: return req.get_response(self.application) except Exception: - LOG.exception(_("FaultWrapper catches error")) + LOG.exception("FaultWrapper catches error") return faults.Fault(webob.exc.HTTPInternalServerError()) @@ -226,7 +226,7 @@ class EC2KeystoneAuth(wsgi.Middleware): auth_ref = keystone_access.AccessInfo.factory(resp=response, body=response.json()) except (NotImplementedError, KeyError): - LOG.exception(_("Keystone failure")) + LOG.exception("Keystone failure") msg = _("Failure communicating with keystone") return faults.ec2_error_response(request_id, "AuthFailure", msg, status=400) diff --git a/ec2api/api/apirequest.py b/ec2api/api/apirequest.py index 0b337356..bcd25855 100644 --- a/ec2api/api/apirequest.py +++ b/ec2api/api/apirequest.py @@ -24,7 +24,7 @@ import six from ec2api.api import cloud from ec2api.api import ec2utils from ec2api import exception -from ec2api.i18n import _ + CONF = cfg.CONF LOG = logging.getLogger(__name__) @@ -52,7 +52,7 @@ class APIRequest(object): method = getattr(self.controller, ec2utils.camelcase_to_underscore(self.action)) except AttributeError: - LOG.exception(_('Unsupported API request: action = %(action)s'), + LOG.exception('Unsupported API request: action = %(action)s', {'action': self.action}) raise exception.InvalidRequest() diff --git a/ec2api/api/ec2utils.py b/ec2api/api/ec2utils.py index f5b34b65..1f0abb6f 100644 --- a/ec2api/api/ec2utils.py +++ b/ec2api/api/ec2utils.py @@ -203,7 +203,7 @@ def is_ec2_timestamp_expired(request, expires=None): timeutils.is_newer_than(query_time, expires)) return False except ValueError: - LOG.exception(_("Timestamp is invalid: ")) + LOG.exception("Timestamp is invalid: ") return True diff --git a/ec2api/api/subnet.py b/ec2api/api/subnet.py index 068de775..ca1beeb2 100644 --- a/ec2api/api/subnet.py +++ b/ec2api/api/subnet.py @@ -127,8 +127,8 @@ def delete_subnet(context, subnet_id): try: neutron.delete_network(os_subnet['network_id']) except neutron_exception.NetworkInUseClient as ex: - LOG.warning(_('Failed to delete network %(os_id)s during ' - 'deleting Subnet %(id)s. Reason: %(reason)s'), + LOG.warning('Failed to delete network %(os_id)s during ' + 'deleting Subnet %(id)s. Reason: %(reason)s', {'id': subnet['id'], 'os_id': os_subnet['network_id'], 'reason': ex.message}) diff --git a/ec2api/api/volume.py b/ec2api/api/volume.py index 64256750..ea8ac649 100644 --- a/ec2api/api/volume.py +++ b/ec2api/api/volume.py @@ -68,7 +68,7 @@ def attach_volume(context, volume_id, instance_id, device): device) except (nova_exception.Conflict, nova_exception.BadRequest): # TODO(andrey-mp): raise correct errors for different cases - LOG.exception(_('Attach has failed.')) + LOG.exception('Attach has failed.') raise exception.UnsupportedOperation() cinder = clients.cinder(context) os_volume = cinder.volumes.get(volume['os_id']) diff --git a/ec2api/api/vpc.py b/ec2api/api/vpc.py index 1babdb70..61847e41 100644 --- a/ec2api/api/vpc.py +++ b/ec2api/api/vpc.py @@ -89,8 +89,8 @@ def delete_vpc(context, vpc_id): try: neutron.delete_router(vpc['os_id']) except neutron_exception.Conflict as ex: - LOG.warning(_('Failed to delete router %(os_id)s during deleting ' - 'VPC %(id)s. Reason: %(reason)s'), + LOG.warning('Failed to delete router %(os_id)s during deleting ' + 'VPC %(id)s. Reason: %(reason)s', {'id': vpc['id'], 'os_id': vpc['os_id'], 'reason': ex.message}) diff --git a/ec2api/api/vpn_connection.py b/ec2api/api/vpn_connection.py index ab5de36c..a5eff386 100644 --- a/ec2api/api/vpn_connection.py +++ b/ec2api/api/vpn_connection.py @@ -165,20 +165,19 @@ def delete_vpn_connection(context, vpn_connection_id): try: neutron.delete_ipsecpolicy(vpn_connection['os_ipsecpolicy_id']) except neutron_exception.Conflict as ex: - LOG.warning( - _('Failed to delete ipsecoplicy %(os_id)s during deleting ' - 'VPN connection %(id)s. Reason: %(reason)s'), - {'id': vpn_connection['id'], - 'os_id': vpn_connection['os_ipsecpolicy_id'], - 'reason': ex.message}) + LOG.warning('Failed to delete ipsecoplicy %(os_id)s during ' + 'deleting VPN connection %(id)s. Reason: %(reason)s', + {'id': vpn_connection['id'], + 'os_id': vpn_connection['os_ipsecpolicy_id'], + 'reason': ex.message}) except neutron_exception.NotFound: pass try: neutron.delete_ikepolicy(vpn_connection['os_ikepolicy_id']) except neutron_exception.Conflict as ex: LOG.warning( - _('Failed to delete ikepolicy %(os_id)s during deleting ' - 'VPN connection %(id)s. Reason: %(reason)s'), + 'Failed to delete ikepolicy %(os_id)s during deleting ' + 'VPN connection %(id)s. Reason: %(reason)s', {'id': vpn_connection['id'], 'os_id': vpn_connection['os_ikepolicy_id'], 'reason': ex.message}) diff --git a/ec2api/api/vpn_gateway.py b/ec2api/api/vpn_gateway.py index a5c207e2..c77fce06 100644 --- a/ec2api/api/vpn_gateway.py +++ b/ec2api/api/vpn_gateway.py @@ -198,8 +198,8 @@ def _safe_delete_vpnservice(neutron, os_vpnservice_id, subnet_id): pass except neutron_exception.Conflict as ex: LOG.warning( - _('Failed to delete vpnservice %(os_id)s for subnet %(id)s. ' - 'Reason: %(reason)s'), + 'Failed to delete vpnservice %(os_id)s for subnet %(id)s. ' + 'Reason: %(reason)s', {'id': subnet_id, 'os_id': os_vpnservice_id, 'reason': ex.message}) diff --git a/ec2api/exception.py b/ec2api/exception.py index 1e36d0d8..02cd7cdb 100644 --- a/ec2api/exception.py +++ b/ec2api/exception.py @@ -59,8 +59,8 @@ class EC2APIException(Exception): exc_info = sys.exc_info() # kwargs doesn't match a variable in the message # log the issue and the kwargs - LOG.exception(_('Exception in string format operation for ' - '%s exception'), self.__class__.__name__) + LOG.exception('Exception in string format operation for ' + '%s exception', self.__class__.__name__) for name, value in kwargs.items(): LOG.error('%s: %s' % (name, value)) @@ -70,8 +70,8 @@ class EC2APIException(Exception): # at least get the core message out if something happened message = self.msg_fmt elif not isinstance(message, six.string_types): - LOG.error(_("Message '%(msg)s' for %(ex)s exception is not " - "a string"), + LOG.error("Message '%(msg)s' for %(ex)s exception is not " + "a string", {'msg': message, 'ex': self.__class__.__name__}) if CONF.fatal_exception_format_errors: raise TypeError(_('Invalid exception message format')) diff --git a/ec2api/hacking/__init__.py b/ec2api/hacking/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/ec2api/hacking/checks.py b/ec2api/hacking/checks.py new file mode 100644 index 00000000..b6d3ad9f --- /dev/null +++ b/ec2api/hacking/checks.py @@ -0,0 +1,52 @@ +# Copyright (c) 2017 OpenStack Foundation. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import re + + +_all_log_levels = {'critical', 'error', 'exception', 'info', + 'warning', 'debug'} + +# Since _Lx() have been removed, we just need to check _() +_all_hints = {'_'} + +_log_translation_hint = re.compile( + r".*LOG\.(%(levels)s)\(\s*(%(hints)s)\(" % { + 'levels': '|'.join(_all_log_levels), + 'hints': '|'.join(_all_hints), + }) + + +def no_translate_logs(logical_line, filename): + """N537 - Don't translate logs. + + Check for 'LOG.*(_(' + + Translators don't provide translations for log messages, and operators + asked not to translate them. + + * This check assumes that 'LOG' is a logger. + + :param logical_line: The logical line to check. + :param filename: The file name where the logical line exists. + :returns: None if the logical line passes the check, otherwise a tuple + is yielded that contains the offending index in logical line and a + message describe the check validation failure. + """ + if _log_translation_hint.match(logical_line): + yield (0, "N537: Log messages should not be translated!") + + +def factory(register): + register(no_translate_logs) diff --git a/ec2api/metadata/api.py b/ec2api/metadata/api.py index 68701d13..10a2bc93 100644 --- a/ec2api/metadata/api.py +++ b/ec2api/metadata/api.py @@ -25,7 +25,7 @@ from ec2api.api import clients from ec2api.api import ec2utils from ec2api.api import instance as instance_api from ec2api import exception -from ec2api.i18n import _ + CONF = cfg.CONF LOG = logging.getLogger(__name__) @@ -137,7 +137,7 @@ def _get_ec2_instance_and_reservation(context, os_instance_id): 'value': [instance_id]}]) if (len(ec2_reservations['reservationSet']) != 1 or len(ec2_reservations['reservationSet'][0]['instancesSet']) != 1): - LOG.error(_('Failed to get metadata for instance id: %s'), + LOG.error('Failed to get metadata for instance id: %s', os_instance_id) raise exception.EC2MetadataNotFound() @@ -152,8 +152,8 @@ def _check_instance_owner(context, os_instance_id, owner_id): # It sends project_id as X-Tenant-ID HTTP header. # We make sure it's correct if context.project_id != owner_id: - LOG.warning(_('Tenant_id %(tenant_id)s does not match tenant_id ' - 'of instance %(instance_id)s.'), + LOG.warning('Tenant_id %(tenant_id)s does not match tenant_id ' + 'of instance %(instance_id)s.', {'tenant_id': context.project_id, 'instance_id': os_instance_id}) raise exception.EC2MetadataNotFound() diff --git a/ec2api/tests/unit/test_hacking.py b/ec2api/tests/unit/test_hacking.py new file mode 100644 index 00000000..3dd79396 --- /dev/null +++ b/ec2api/tests/unit/test_hacking.py @@ -0,0 +1,29 @@ +# Copyright (c) 2017 OpenStack Foundation. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from ec2api.hacking import checks +from ec2api.tests.unit import base + + +class HackingTestCase(base.BaseTestCase): + def test_no_log_translations(self): + for log in checks._all_log_levels: + for hint in checks._all_hints: + bad = 'LOG.%s(%s("Bad"))' % (log, hint) + self.assertEqual( + 1, len(list(checks.no_translate_logs(bad, 'f')))) + # Catch abuses when used with a variable and not a literal + bad = 'LOG.%s(%s(msg))' % (log, hint) + self.assertEqual( + 1, len(list(checks.no_translate_logs(bad, 'f')))) diff --git a/ec2api/utils.py b/ec2api/utils.py index cf4a4673..c95c1ae9 100644 --- a/ec2api/utils.py +++ b/ec2api/utils.py @@ -25,7 +25,6 @@ from xml.sax import saxutils from oslo_config import cfg from oslo_log import log as logging -from ec2api.i18n import _ utils_opts = [ cfg.StrOpt('tempdir', @@ -49,7 +48,7 @@ def tempdir(**kwargs): try: shutil.rmtree(tmpdir) except OSError as e: - LOG.error(_('Could not remove tmpdir: %s'), str(e)) + LOG.error('Could not remove tmpdir: %s', str(e)) def get_hash_str(base_str): diff --git a/ec2api/wsgi.py b/ec2api/wsgi.py index 8e1b47cb..961dae35 100644 --- a/ec2api/wsgi.py +++ b/ec2api/wsgi.py @@ -124,12 +124,12 @@ class Server(ServiceBase): try: self._socket = eventlet.listen(bind_addr, family, backlog=backlog) except EnvironmentError: - LOG.error(_("Could not bind to %(host)s:%(port)s"), + LOG.error("Could not bind to %(host)s:%(port)s", {'host': host, 'port': port}) raise (self.host, self.port) = self._socket.getsockname()[0:2] - LOG.info(_("%(name)s listening on %(host)s:%(port)s"), + LOG.info("%(name)s listening on %(host)s:%(port)s", {'name': self.name, 'host': self.host, 'port': self.port}) def start(self): @@ -184,8 +184,8 @@ class Server(ServiceBase): **ssl_kwargs) except Exception: with excutils.save_and_reraise_exception(): - LOG.error(_("Failed to start %(name)s on %(host)s" - ":%(port)s with SSL support"), + LOG.error("Failed to start %(name)s on %(host)s" + ":%(port)s with SSL support", {'name': self.name, 'host': self.host, 'port': self.port}) @@ -222,7 +222,7 @@ class Server(ServiceBase): :returns: None """ - LOG.info(_("Stopping WSGI server.")) + LOG.info("Stopping WSGI server.") if self._server is not None: # Resize pool to stop new requests from being processed @@ -241,7 +241,7 @@ class Server(ServiceBase): if self._server is not None: self._server.wait() except greenlet.GreenletExit: - LOG.info(_("WSGI server has stopped.")) + LOG.info("WSGI server has stopped.") class Request(webob.Request): @@ -499,7 +499,7 @@ class Loader(object): """ try: - LOG.debug(_("Loading app %(name)s from %(path)s") % + LOG.debug("Loading app %(name)s from %(path)s", {'name': name, 'path': self.config_path}) return deploy.loadapp("config:%s" % self.config_path, name=name) except LookupError as err: diff --git a/tox.ini b/tox.ini index 18387852..519c99d1 100644 --- a/tox.ini +++ b/tox.ini @@ -54,7 +54,7 @@ exclude = .venv,.git,.tox,dist,doc,*lib/python*,*egg,build,tools,ec2api/tests/f max-complexity=25 [hacking] -import_exceptions = ec2api.i18n +local-check-factory = ec2api.hacking.checks.factory [testenv:install-guide] commands = sphinx-build -a -E -W -d install-guide/build/doctrees -b html install-guide/source install-guide/build/html