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(