From 7b71585aa1e4bb116ce183f6e2879a1951738fc6 Mon Sep 17 00:00:00 2001 From: Ngo Quoc Cuong Date: Thu, 29 Jun 2017 04:04:26 -0400 Subject: [PATCH] Update log translation hacking rule Starting with the Pike series, OpenStack no longer supports log translation. Update hacking rule to prevent log translation in all log level instead of only debug level. Since all log translations are invalidated, check_explicit_underscore_import can be simplified. Change-Id: I1105fcb7e25549017ed7fcad8f932abc1b635129 Related-Bug: #1701195 --- HACKING.rst | 2 +- cinder/hacking/checks.py | 24 +++++++++++------------- cinder/tests/unit/test_hacking.py | 20 +++++++++++++------- 3 files changed, 25 insertions(+), 21 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 1add95e21be..d33b9d3717c 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -8,7 +8,6 @@ Cinder Style Commandments Cinder Specific Commandments ---------------------------- - [N314] Check for vi editor configuration in source files. -- [N319] Validate that debug level logs are not translated. - [N322] Ensure default arguments are not mutable. - [N323] Add check for explicit import of _() to ensure proper translation. - [N325] str() and unicode() cannot be used on an exception. Remove or use six.text_type(). @@ -24,6 +23,7 @@ Cinder Specific Commandments - [C309] Unit tests should not perform logging. - [C310] Check for improper use of logging format arguments. - [C311] Check for proper naming and usage in option registration. +- [C312] Validate that logs are not translated. - [C313] Check that assertTrue(value) is used and not assertEqual(True, value). General diff --git a/cinder/hacking/checks.py b/cinder/hacking/checks.py index 6bbb428f851..cb38bf5a415 100644 --- a/cinder/hacking/checks.py +++ b/cinder/hacking/checks.py @@ -36,7 +36,7 @@ UNDERSCORE_IMPORT_FILES = ['cinder/objects/__init__.py', 'cinder/objects/manageableresources.py'] translated_log = re.compile( - r"(.)*LOG\.(audit|error|info|warn|warning|critical|exception)" + r"(.)*LOG\.(audit|debug|error|info|warn|warning|critical|exception)" "\(\s*_\(\s*('|\")") string_translation = re.compile(r"(.)*_\(\s*('|\")") vi_header_re = re.compile(r"^#\s+vim?:.+") @@ -123,21 +123,20 @@ def no_vi_headers(physical_line, line_number, lines): return 0, "N314: Don't put vi configuration in source files" -def no_translate_debug_logs(logical_line, filename): - """Check for 'LOG.debug(_(' +def no_translate_logs(logical_line, filename): + """Check for 'LOG.*(_(' - As per our translation policy, - https://wiki.openstack.org/wiki/LoggingStandards#Log_Translation - we shouldn't translate debug level logs. + Starting with the Pike series, OpenStack no longer supports log + translation. We shouldn't translate logs. - This check assumes that 'LOG' is a logger. - Use filename so we can start enforcing this in specific folders instead of needing to do so all at once. - N319 + C312 """ - if logical_line.startswith("LOG.debug(_("): - yield(0, "N319 Don't translate debug level logs") + if translated_log.match(logical_line): + yield(0, "C312: Log messages should not be translated!") def no_mutable_default_args(logical_line): @@ -151,7 +150,7 @@ def check_explicit_underscore_import(logical_line, filename): """Check for explicit import of the _ function We need to ensure that any files that are using the _() function - to translate logs are explicitly importing the _ function. We + to translate messages are explicitly importing the _ function. We can't trust unit test to catch whether the import has been added so we need to check for it here. """ @@ -165,8 +164,7 @@ def check_explicit_underscore_import(logical_line, filename): underscore_import_check_multi.match(logical_line) or custom_underscore_check.match(logical_line)): UNDERSCORE_IMPORT_FILES.append(filename) - elif(translated_log.match(logical_line) or - string_translation.match(logical_line)): + elif string_translation.match(logical_line): yield(0, "N323: Found use of _() without explicit import of _ !") @@ -457,7 +455,7 @@ def validate_assertTrue(logical_line): def factory(register): register(no_vi_headers) - register(no_translate_debug_logs) + register(no_translate_logs) register(no_mutable_default_args) register(check_explicit_underscore_import) register(CheckForStrUnicodeExc) diff --git a/cinder/tests/unit/test_hacking.py b/cinder/tests/unit/test_hacking.py index 3775cafb047..959644e1930 100644 --- a/cinder/tests/unit/test_hacking.py +++ b/cinder/tests/unit/test_hacking.py @@ -78,15 +78,21 @@ class HackingTestCase(test.TestCase): "# vim: et tabstop=4 shiftwidth=4 softtabstop=4", 6, lines)) - def test_no_translate_debug_logs(self): - self.assertEqual(1, len(list(checks.no_translate_debug_logs( + def test_no_translate_logs(self): + self.assertEqual(1, len(list(checks.no_translate_logs( + "LOG.audit(_('foo'))", "cinder/scheduler/foo.py")))) + self.assertEqual(1, len(list(checks.no_translate_logs( "LOG.debug(_('foo'))", "cinder/scheduler/foo.py")))) - - self.assertEqual(0, len(list(checks.no_translate_debug_logs( - "LOG.debug('foo')", "cinder/scheduler/foo.py")))) - - self.assertEqual(0, len(list(checks.no_translate_debug_logs( + self.assertEqual(1, len(list(checks.no_translate_logs( + "LOG.error(_('foo'))", "cinder/scheduler/foo.py")))) + self.assertEqual(1, len(list(checks.no_translate_logs( "LOG.info(_('foo'))", "cinder/scheduler/foo.py")))) + self.assertEqual(1, len(list(checks.no_translate_logs( + "LOG.warning(_('foo'))", "cinder/scheduler/foo.py")))) + self.assertEqual(1, len(list(checks.no_translate_logs( + "LOG.exception(_('foo'))", "cinder/scheduler/foo.py")))) + self.assertEqual(1, len(list(checks.no_translate_logs( + "LOG.critical(_('foo'))", "cinder/scheduler/foo.py")))) def test_check_explicit_underscore_import(self): self.assertEqual(1, len(list(checks.check_explicit_underscore_import(