Merge "Enforce log messages not being translated"

HACKING.rst

@@ -17,7 +17,7 @@ Neutron Library Specific Commandments
 - [N529] Method's default argument shouldn't be mutable
 - [N530] No importing of neutron; should be ignored in neutron itself
 - [N532] Validate that LOG.warning is used instead of LOG.warn. The latter is deprecated.
-- [N533] Validate that debug level logs are not translated
 - [N534] Exception messages should be translated
 - [N535] Usage of Python eventlet module not allowed
 - [N536] Use assertIsNone/assertIsNotNone rather than assertEqual/assertIs to check None values.
+- [N537] Don't translate logs.

neutron_lib/_i18n.py

@@ -20,15 +20,5 @@ import oslo_i18n
 
 _translators = oslo_i18n.TranslatorFactory(domain='neutron_lib')
 
-# The primary translation function using the well-known name "_"
+# The translation function using the well-known name "_"
 _ = _translators.primary
-
-# Translators for log levels.
-#
-# The abbreviated names are meant to reflect the usual use of a short
-# name like '_'. The "L" is for "log" and the other letter comes from
-# the level.
-_LI = _translators.log_info
-_LW = _translators.log_warning
-_LE = _translators.log_error
-_LC = _translators.log_critical

neutron_lib/callbacks/manager.py

@@ -15,7 +15,6 @@ import collections
 from oslo_log import log as logging
 from oslo_utils import reflection
 
-from neutron_lib._i18n import _LE
 from neutron_lib.callbacks import events
 from neutron_lib.callbacks import exceptions
 from neutron_lib.db import utils as db_utils
@@ -176,8 +175,8 @@ class CallbacksManager(object):
                     event.startswith(events.PRECOMMIT)
                 )
                 if not abortable_event:
-                    LOG.exception(_LE("Error during notification for "
+                    LOG.exception("Error during notification for "
-                                      "%(callback)s %(resource)s, %(event)s"),
+                                  "%(callback)s %(resource)s, %(event)s",
                                   {'callback': callback_id,
                                    'resource': resource, 'event': event})
                 else:

neutron_lib/hacking/checks.py

@@ -257,7 +257,7 @@ def factory(register):
     register(check_python3_no_iteritems)
     register(no_mutable_default_args)
     register(check_neutron_namespace_imports)
-    register(translation_checks.no_translate_debug_logs)
+    register(translation_checks.no_translate_logs)
     register(translation_checks.check_log_warn_deprecated)
     register(translation_checks.check_raised_localized_exceptions)
     register(assert_equal_none)

neutron_lib/hacking/translation_checks.py

@@ -15,14 +15,22 @@
 import re
 
 
+_all_log_levels = {'critical', 'error', 'exception', 'info', 'warning'}
 _all_hints = {'_LC', '_LE', '_LI', '_', '_LW'}
 
 
 _log_warn = re.compile(
-    r"(.)*LOG\.(warn)\(\s*('|\"|_)")
+    r"(.)*LOG\.(warn)\(\s*('|\")")
 
 
-def _translation_is_not_expected(filename):
+_log_translation_hint = re.compile(
+    r".*LOG\.(%(levels)s)\(\s*(%(hints)s)\(" % {
+        'levels': '|'.join(_all_log_levels),
+        'hints': '|'.join(_all_hints),
+    })
+
+
+def _translation_checks_not_enforced(filename):
     # Do not do these validations on tests
     return any(pat in filename for pat in ["/tests/", "rally-jobs/plugins/"])
 
@@ -41,28 +49,6 @@ def check_log_warn_deprecated(logical_line, filename):
         yield (0, msg)
 
 
-def no_translate_debug_logs(logical_line, filename):
-    """N533 - Don't translate debug level logs.
-
-    Check for 'LOG.debug(_(' and 'LOG.debug(_Lx('
-
-    As per our translation policy,
-    https://wiki.openstack.org/wiki/LoggingStandards#Log_Translation
-    we shouldn't translate debug level logs.
-
-    * 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.
-    """
-    for hint in _all_hints:
-        if logical_line.startswith("LOG.debug(%s(" % hint):
-            yield(0, "N533 Don't translate debug level logs")
-
-
 def check_raised_localized_exceptions(logical_line, filename):
     """N534 - Untranslated exception message.
 
@@ -72,7 +58,7 @@ def check_raised_localized_exceptions(logical_line, filename):
     is yielded that contains the offending index in logical line and a
     message describe the check validation failure.
     """
-    if _translation_is_not_expected(filename):
+    if _translation_checks_not_enforced(filename):
         return
 
     logical_line = logical_line.strip()
@@ -83,3 +69,28 @@ def check_raised_localized_exceptions(logical_line, filename):
         if exception_msg.startswith("\"") or exception_msg.startswith("\'"):
             msg = "N534: Untranslated exception message."
             yield (logical_line.index(exception_msg), msg)
+
+
+def no_translate_logs(logical_line, filename):
+    """N537 - Don't translate logs.
+
+    Check for 'LOG.*(_(' and 'LOG.*(_Lx('
+
+    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 _translation_checks_not_enforced(filename):
+        return
+
+    msg = "N537: Log messages should not be translated!"
+    match = _log_translation_hint.match(logical_line)
+    if match:
+        yield (logical_line.index(match.group()), msg)

neutron_lib/tests/unit/hacking/test_checks.py

@@ -134,15 +134,24 @@ class HackingTestCase(base.BaseTestCase):
         self.assertLineFails(f, 'from neutron import context')
         self.assertLineFails(f, 'import neutron.common.config')
 
-    def test_no_translate_debug_logs(self):
+    def test_no_log_translations(self):
+        for log in tc._all_log_levels:
             for hint in tc._all_hints:
-            bad = "LOG.debug(%s('bad'))" % hint
+                bad = 'LOG.%s(%s("Bad"))' % (log, hint)
                 self.assertEqual(
-                1, len(list(tc.no_translate_debug_logs(bad, 'f'))))
+                    1, len(list(tc.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(tc.no_translate_logs(bad, 'f'))))
+                # Do not do validations in tests
+                ok = 'LOG.%s(_("OK - unit tests"))' % log
+                self.assertEqual(
+                    0, len(list(tc.no_translate_logs(ok, 'f/tests/f'))))
 
     def test_check_log_warn_deprecated(self):
-        bad = "LOG.warn(_LW('i am deprecated!'))"
+        bad = "LOG.warn('i am deprecated!')"
-        good = "LOG.warning(_LW('zlatan is the best'))"
+        good = "LOG.warning('zlatan is the best')"
         f = tc.check_log_warn_deprecated
         self.assertLineFails(f, bad, '')
         self.assertLinePasses(f, good, '')

releasenotes/notes/new-hacking-check-no-log-translations-4a430a38aeb06452.yaml

@@ -0,0 +1,14 @@
+---
+features:
+  - |
+    New ``N537`` hacking check is introduced that enforces no logging message
+    translations, in any logging level. The check is enabled by default. Also,
+    the ``N533`` hacking check is now removed because it is covered by
+    ``N537``.
+upgrade:
+  - |
+    Library consumers may need to adopt their code to new requirements of ``N537`` hacking check,
+    removing translation markers from all logging messages. If for some reason
+    it doesn't fit the project, consumers can disable the new hacking check
+    using ``ignore`` statement in ``flake8`` section of their ``tox.ini`` file,
+    or by other means.