From adf268def3b9764b3ece37db41255b78515257c4 Mon Sep 17 00:00:00 2001 From: Markus Zoeller Date: Tue, 24 Nov 2015 17:37:13 +0100 Subject: [PATCH] add hacking check for config options location This adds a hackign check which raises a pep8 error if a config option was found in location where it shouldn't be. Change-Id: I75df3404cc6178aa179e96d75f154e389c3008af --- HACKING.rst | 1 + nova/hacking/checks.py | 27 ++++++++++++++++++++++ nova/tests/unit/test_hacking.py | 40 +++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+) diff --git a/HACKING.rst b/HACKING.rst index 5b769c1933..881d8de006 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -52,6 +52,7 @@ Nova Specific Commandments - [N339] Check common raise_feature_not_supported() is used for v2.1 HTTPNotImplemented response. - [N340] Check nova.utils.spawn() is used instead of greenthread.spawn() and eventlet.spawn() - [N341] contextlib.nested is deprecated +- [N342] Config options should be in the central location ``nova/conf/`` Creating Unit Tests ------------------- diff --git a/nova/hacking/checks.py b/nova/hacking/checks.py index 4f1400ad15..8d1237299e 100644 --- a/nova/hacking/checks.py +++ b/nova/hacking/checks.py @@ -36,6 +36,8 @@ UNDERSCORE_IMPORT_FILES = [] session_check = re.compile(r"\w*def [a-zA-Z0-9].*[(].*session.*[)]") cfg_re = re.compile(r".*\scfg\.") +# Excludes oslo.config OptGroup objects +cfg_opt_re = re.compile(r".*[\s\[]cfg\.[a-zA-Z]*Opt\(") vi_header_re = re.compile(r"^#\s+vim?:.+") virt_file_re = re.compile(r"\./nova/(?:tests/)?virt/(\w+)/") virt_import_re = re.compile( @@ -550,6 +552,30 @@ def check_no_contextlib_nested(logical_line, filename): yield(0, msg) +def check_config_option_in_central_place(logical_line, filename): + msg = ("N342: Config options should be in the central location " + "'/nova/conf/*'. Do not declare new config options outside " + "of that folder.") + # That's the correct location + if "nova/conf/" in filename: + return + # TODO(markus_z) This is just temporary until all config options are + # moved to the central place. To avoid that a once cleaned up place + # introduces new config options, we do a check here. This array will + # get quite huge over the time, but will be removed at the end of the + # reorganization. + # You can add the full path to a module or folder. It's just a substring + # check, which makes it flexible enough. + cleaned_up = ["nova/console/serial.py", + "nova/cmd/serialproxy.py", + ] + if not any(c in filename for c in cleaned_up): + return + + if cfg_opt_re.match(logical_line): + yield(0, msg) + + def factory(register): register(import_no_db_in_virt) register(no_db_session_in_public_api) @@ -578,3 +604,4 @@ def factory(register): register(check_http_not_implemented) register(check_no_contextlib_nested) register(check_greenthread_spawns) + register(check_config_option_in_central_place) diff --git a/nova/tests/unit/test_hacking.py b/nova/tests/unit/test_hacking.py index d8f3f8fac9..0469095e51 100644 --- a/nova/tests/unit/test_hacking.py +++ b/nova/tests/unit/test_hacking.py @@ -588,3 +588,43 @@ class HackingTestCase(test.NoDBTestCase): code = "nova.utils.spawn_n(func, arg1, kwarg1=kwarg1)" self._assert_has_no_errors(code, checks.check_greenthread_spawns) + + def test_config_option_regex_match(self): + def should_match(code): + self.assertTrue(checks.cfg_opt_re.match(code)) + + def should_not_match(code): + self.assertFalse(checks.cfg_opt_re.match(code)) + + should_match("opt = cfg.StrOpt('opt_name')") + should_match("opt = cfg.IntOpt('opt_name')") + should_match("opt = cfg.DictOpt('opt_name')") + should_match("opt = cfg.Opt('opt_name')") + should_match("opts=[cfg.Opt('opt_name')]") + should_match(" cfg.Opt('opt_name')") + should_not_match("opt_group = cfg.OptGroup('opt_group_name')") + + def test_check_config_option_in_central_place(self): + errors = [(1, 0, "N342")] + code = """ + opts = [ + cfg.StrOpt('random_opt', + default='foo', + help='I am here to do stuff'), + ] + """ + # option at the right place in the tree + self._assert_has_no_errors(code, + checks.check_config_option_in_central_place, + filename="nova/conf/serial_console.py") + # option at a location which is not in scope right now + # TODO(markus_z): This is remporary until all config options are + # moved to /nova/conf + self._assert_has_no_errors(code, + checks.check_config_option_in_central_place, + filename="nova/dummy/non_existent.py") + # option at the wrong place in the tree + self._assert_has_errors(code, + checks.check_config_option_in_central_place, + filename="nova/cmd/serialproxy.py", + expected_errors=errors)