From 391d18dc640de24d65662444c1c022244b9a6ed5 Mon Sep 17 00:00:00 2001 From: Kendall Nelson Date: Mon, 14 Sep 2015 20:37:00 -0500 Subject: [PATCH] Hacking check for opt name registration Depending on how opts are registered (either with register_opt() or register_opts()), the name needs to be singular or plural to match the method. This patch adds a hacking check to make sure the names of the opts and opt lists (or tuples) are correct given how they are being registered. The check also verifies that a single option is sent when register_opt() is used and a list is used when using register_opts(). Includes fixes to files that don't meet the naming convention and a addition to the generate_cinder_opts.py file in order to skip checks.py in the generation of the opts.py file. Closes-Bug: 1495752 Change-Id: Ia795915c6e3d46272acc30407961d5d876f783ce --- HACKING.rst | 1 + cinder/config/generate_cinder_opts.py | 3 +- cinder/hacking/checks.py | 79 +++++++++++++++++++++++++++ cinder/image/glance.py | 4 +- cinder/tests/unit/test_hacking.py | 32 +++++++++++ 5 files changed, 116 insertions(+), 3 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 3b0d67285..fc65b8cfd 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -27,6 +27,7 @@ Cinder Specific Commandments - [C308] timeutils.isotime() must not be used (deprecated). - [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. General ------- diff --git a/cinder/config/generate_cinder_opts.py b/cinder/config/generate_cinder_opts.py index 50c206bd0..2a57b0d1f 100644 --- a/cinder/config/generate_cinder_opts.py +++ b/cinder/config/generate_cinder_opts.py @@ -49,7 +49,8 @@ if __name__ == "__main__": for atree in dir_trees_list: - if atree == "cinder/config/generate_cinder_opts.py": + if atree in ["cinder/config/generate_cinder_opts.py", + "cinder/hacking/checks.py"]: continue dirs_list = atree.split('/') diff --git a/cinder/hacking/checks.py b/cinder/hacking/checks.py index ea27decf7..a50f17f36 100644 --- a/cinder/hacking/checks.py +++ b/cinder/hacking/checks.py @@ -268,6 +268,84 @@ class CheckLoggingFormatArgs(BaseASTChecker): return super(CheckLoggingFormatArgs, self).generic_visit(node) +class CheckOptRegistrationArgs(BaseASTChecker): + """Verifying the registration of options are well formed + + This class creates a check for single opt or list/tuple of + opts when register_opt() or register_opts() are being called. + """ + + CHECK_DESC = ('C311: Arguments being passed to register_opt/register_opts ' + 'must be a single option or list/tuple of options ' + 'respectively. Options must also end with _opt or _opts ' + 'respectively.') + + singular_method = 'register_opt' + plural_method = 'register_opts' + + register_methods = [ + singular_method, + plural_method, + ] + + def _find_name(self, node): + """Return the fully qualified name or a Name or Attribute.""" + if isinstance(node, ast.Name): + return node.id + elif (isinstance(node, ast.Attribute) + and isinstance(node.value, (ast.Name, ast.Attribute))): + method_name = node.attr + obj_name = self._find_name(node.value) + if obj_name is None: + return None + return obj_name + '.' + method_name + elif isinstance(node, six.string_types): + return node + else: # could be Subscript, Call or many more + return None + + def visit_Call(self, node): + """Look for the register_opt/register_opts calls.""" + # extract the obj_name and method_name + if isinstance(node.func, ast.Attribute): + if not isinstance(node.func.value, ast.Name): + return (super(CheckOptRegistrationArgs, + self).generic_visit(node)) + + method_name = node.func.attr + + # obj must be instance of register_opt() or register_opts() + if method_name not in self.register_methods: + return (super(CheckOptRegistrationArgs, + self).generic_visit(node)) + + # argument should be a list with register_opts() + if len(node.args) > 0: + argument_name = self._find_name(node.args[0]) + if argument_name: + if (method_name == self.singular_method and + not argument_name.lower().endswith('opt')): + self.add_error(node.args[0]) + elif (method_name == self.plural_method and + not argument_name.lower().endswith('opts')): + self.add_error(node.args[0]) + else: + # This covers instances of register_opt()/register_opts() + # that are registering the objects directly and not + # passing in a variable referencing the options being + # registered. + if (method_name == self.singular_method and + (isinstance(node.args[0], ast.List)) or + isinstance(node.args[0], ast.Tuple)): + self.add_error(node.args[0]) + elif (method_name == self.plural_method and + (not isinstance(node.args[0], ast.List)) or + isinstance(node.args[0], ast.Tuple)): + self.add_error(node.args[0]) + + return super(CheckOptRegistrationArgs, self).generic_visit(node) + + def validate_log_translations(logical_line, filename): # Translations are not required in the test directory. # This will not catch all instances of violations, just direct @@ -393,6 +471,7 @@ def factory(register): register(check_explicit_underscore_import) register(CheckForStrUnicodeExc) register(CheckLoggingFormatArgs) + register(CheckOptRegistrationArgs) register(check_oslo_namespace_imports) register(check_datetime_now) register(check_timeutils_strtime) diff --git a/cinder/image/glance.py b/cinder/image/glance.py index 444fb50c6..d0785718f 100644 --- a/cinder/image/glance.py +++ b/cinder/image/glance.py @@ -46,7 +46,7 @@ glance_opts = [ 'via the direct_url. Currently supported schemes: ' '[file].'), ] -glance_core_properties = [ +glance_core_properties_opts = [ cfg.ListOpt('glance_core_properties', default=['checksum', 'container_format', 'disk_format', 'image_name', 'image_id', @@ -55,7 +55,7 @@ glance_core_properties = [ ] CONF = cfg.CONF CONF.register_opts(glance_opts) -CONF.register_opts(glance_core_properties) +CONF.register_opts(glance_core_properties_opts) CONF.import_opt('glance_api_version', 'cinder.common.config') LOG = logging.getLogger(__name__) diff --git a/cinder/tests/unit/test_hacking.py b/cinder/tests/unit/test_hacking.py index 334d81eaa..5502f949b 100644 --- a/cinder/tests/unit/test_hacking.py +++ b/cinder/tests/unit/test_hacking.py @@ -175,6 +175,38 @@ class HackingTestCase(test.TestCase): self._assert_has_errors(code, checker, expected_errors=[(4, 37, 'C310')]) + def test_opt_type_registration_args(self): + checker = checks.CheckOptRegistrationArgs + code = """ + CONF.register_opts([opt1, opt2, opt3]) + CONF.register_opt(lonely_opt) + CONF.register_opts([OPT1, OPT2], group="group_of_opts") + CONF.register_opt(single_opt, group=blah) + """ + self._assert_has_no_errors(code, checker) + + code = """ + CONF.register_opt([opt4, opt5, opt6]) + CONF.register_opts(lonely_opt) + CONF.register_opt((an_opt, another_opt)) + """ + for method in checker.register_methods: + self._assert_has_errors(code.format(method), checker, + expected_errors=[(1, 18, 'C311'), + (2, 19, 'C311'), + (3, 19, 'C311')]) + + code = """ + CONF.register_opt(single_opt) + CONF.register_opts(other_opt) + CONF.register_opt(multiple_opts) + tuple_opts = (one_opt, two_opt) + CONF.register_opts(tuple_opts) + """ + self._assert_has_errors(code, checker, + expected_errors=[(2, 19, 'C311'), + (3, 18, 'C311')]) + def test_str_unicode_exception(self): checker = checks.CheckForStrUnicodeExc -- 2.45.2