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

diff --git a/HACKING.rst b/HACKING.rst
index 3b0d67285a061ab5b8530a1ad8bceefe4b93c45b..fc65b8cfda99710a13d5d87966a07b6fc77159ed 100644 (file)
--- 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 50c206bd0cb54aac0ce423a87359d259b15be1bf..2a57b0d1f58a157d2d84efd32236b1720e11da02 100644 (file)
--- 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 ea27decf7ad3520ef420c7a32a5d7180b83b00cc..a50f17f367bf8d272d868632ea7ddc902b658d1d 100644 (file)
--- 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 444fb50c69a881be0f907fd09907c8534eba4477..d0785718f7538d450451f02a4ebc9d99a0f94f3f 100644 (file)
--- 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 334d81eaa2c63dc6f9685d474c9e07520d0a25bf..5502f949b66e1c312021717acd7770818da26916 100644 (file)
--- 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