Merge pull request #7 from rezib/pr/conf-struct

Restructure vm and gerrit configuration parameters

Author: valeriyoann

lib/rift/Config.py

@@ -35,6 +35,9 @@
 """
 import errno
 import os
+import warnings
+import logging
+
 import yaml
 
 from rift import DeclError
@@ -56,6 +59,9 @@ def _construct_mapping(loader, node):
     loader.flatten_mapping(node)
     return OrderedDict(loader.construct_pairs(node))
 
+class RiftDeprecatedConfWarning(FutureWarning):
+    """Warning emitted when deprecated configuration parameter is loaded."""
+
 OrderedLoader.add_constructor(
     yaml.resolver.BaseResolver.DEFAULT_MAPPING_TAG,
     _construct_mapping)
@@ -175,55 +181,114 @@ class Config():
                 }
             }
         },
-        'vm_image':    {
+        'vm': {
+            'check':    'dict',
             'required': True,
-            # XXX?: default value?
+            'syntax': {
+                'image':    {
+                    'required': True,
+                    # XXX?: default value?
+                },
+                'image_copy':    {
+                    'check':    'digit',
+                    'default': 0,
+                },
+                'port_range': {
+                    'check':    'dict',
+                    'syntax': {
+                        'min': {
+                            'check': 'digit',
+                            'default': _DEFAULT_VM_PORT_RANGE_MIN,
+                        },
+                        'max': {
+                            'check': 'digit',
+                            'default': _DEFAULT_VM_PORT_RANGE_MAX,
+                        }
+                    }
+                },
+                'cpu': {},
+                'cpus': {
+                    'check':    'digit',
+                    'default':  _DEFAULT_VM_CPUS,
+                },
+                'memory': {
+                    'check':    'digit',
+                    'default':   _DEFAULT_VM_MEMORY,
+                },
+                'address': {
+                    'default':  _DEFAULT_VM_ADDRESS,
+                },
+                'images_cache': {},
+                'additional_rpms': {
+                    'check':    'list',
+                    'default':  _DEFAULT_VM_ADDITIONAL_RPMS,
+                },
+                'cloud_init_tpl': {
+                    'default': _DEFAULT_VM_CLOUD_INIT_TPL,
+                },
+                'build_post_script': {
+                    'default': _DEFAULT_VM_BUILD_POST_SCRIPT,
+                },
+            }
+        },
+        'vm_image':    {
+            'deprecated': 'vm.image'
         },
         'vm_image_copy':    {
-            'check':    'digit',
-            'default': 0,
+            'deprecated': 'vm.image_copy'
         },
         'vm_port_range': {
-            'check':    'dict',
-            'syntax': {
-                'min': {
-                    'check': 'digit',
-                    'default': _DEFAULT_VM_PORT_RANGE_MIN,
-                },
-                'max': {
-                    'check': 'digit',
-                    'default': _DEFAULT_VM_PORT_RANGE_MAX,
-                }
-            }
+            'deprecated': 'vm.port_range'
+        },
+        'vm_cpu': {
+            'deprecated': 'vm.cpu'
         },
-        'vm_cpu': {},
         'vm_cpus': {
-            'check':    'digit',
-            'default':  _DEFAULT_VM_CPUS,
+            'deprecated': 'vm.cpus'
         },
         'vm_memory': {
-            'check':    'digit',
-            'default':   _DEFAULT_VM_MEMORY,
+            'deprecated': 'vm.memory'
         },
         'vm_address': {
-            'default':  _DEFAULT_VM_ADDRESS,
+            'deprecated': 'vm.address'
+        },
+        'vm_images_cache': {
+            'deprecated': 'vm.images_cache'
         },
-        'vm_images_cache': {},
         'vm_additional_rpms': {
-            'check':    'list',
-            'default':  _DEFAULT_VM_ADDITIONAL_RPMS,
+            'deprecated': 'vm.additional_rpms'
         },
         'vm_cloud_init_tpl': {
-            'default': _DEFAULT_VM_CLOUD_INIT_TPL,
+            'deprecated': 'vm.cloud_init_tpl'
         },
         'vm_build_post_script': {
-            'default': _DEFAULT_VM_BUILD_POST_SCRIPT,
+            'deprecated': 'vm.build_post_script'
+        },
+        'gerrit': {
+            'check': 'dict',
+            'syntax': {
+                'realm': {},
+                'server': {},
+                'url': {},
+                'username': {},
+                'password': {},
+            }
+        },
+        'gerrit_realm': {
+            'deprecated': 'gerrit.realm'
+        },
+        'gerrit_server': {
+            'deprecated': 'gerrit.server'
+        },
+        'gerrit_url': {
+            'deprecated': 'gerrit.url'
+        },
+        'gerrit_username': {
+            'deprecated': 'gerrit.username'
+        },
+        'gerrit_password': {
+            'deprecated': 'gerrit.password'
         },
-        'gerrit_realm': {},
-        'gerrit_server': {},
-        'gerrit_url': {},
-        'gerrit_username': {},
-        'gerrit_password': {},
         'rpm_macros': {
             'check':    'dict',
         },
@@ -444,6 +509,12 @@ def set(self, key, value, arch=None):
         if key not in self.SYNTAX:
             raise DeclError(f"Unknown '{key}' key")
 
+        # Check not deprecated
+        replacement = self.SYNTAX[key].get('deprecated')
+        if replacement:
+            raise DeclError(f"Parameter {key} is deprecated, use "
+                            f"{' > '.join(replacement.split('.'))} instead")
+
         options = self._arch_options(arch)
         value = self._key_value(
             self.SYNTAX[key],
@@ -522,8 +593,8 @@ def _dict_value(self, syntax, key, value):
             raise DeclError(f"Unknown {key} keys: {', '.join(unknown_keys)}")
 
         # Iterate over the keys defined in syntax. If the subvalue or default
-        # value is defined, set it or raise error if required.
-        for subkey, subkey_format in syntax.items():
+        # value is defined, set it.
+        for subkey in syntax.keys():
             subkey_value = value.get(subkey,
                                      Config._syntax_default(syntax, subkey))
             if subkey_value is not None:
@@ -533,12 +604,7 @@ def _dict_value(self, syntax, key, value):
                     subkey_value,
                     syntax[subkey].get('check', 'string')
                 )
-            elif subkey_format.get('required', False):
-                raise DeclError(
-                    f"Key {subkey} is required in dict parameter {key}"
-                )
 
-        # Set the key in options dict eventually.
         return result
 
     def _record_value(self, syntax, value):
@@ -556,6 +622,73 @@ def _record_value(self, syntax, value):
             )
         return result
 
+    @staticmethod
+    def _get_replacement_dict_key(data, replacement):
+        """
+        Return a 2-tuple with the dict that contains the replacement parameter
+        and the key of this parameter in this dict.
+        """
+        sub = data
+        replacement_items = replacement.split('.')
+        # Browse in data dict depth until last replacement item.
+        for index, item in enumerate(replacement_items, start=1):
+            if index < len(replacement_items):
+                if item not in sub:
+                    sub[item] = {}
+                sub = sub[item]
+            else:
+                return sub, item
+        return None
+
+    def _move_deprecated_param(self, data, param, value):
+        """
+        If the given parameter is deprecated, move its value to its replacement
+        parameter.
+        """
+        # Leave if parameter not found in syntax, the error is managed in set()
+        # method eventually.
+        if param not in self.SYNTAX:
+            return
+        # Check presence of deprecated attribute and leave if not defined.
+        replacement = self.SYNTAX[param].get("deprecated")
+        if replacement is None:
+            return
+        # Warn user with FutureWarning.
+        warnings.warn(f"Configuration parameter {param} is deprecated, use "
+                      f"{' > '.join(replacement.split('.'))} instead",
+                      RiftDeprecatedConfWarning)
+        # Get position of replacement parameter.
+        sub, item = Config._get_replacement_dict_key(data, replacement)
+        # If both new and deprecated parameter are defined, emit warning log to
+        # explain deprecated parameter is ignored. Else, move deprecated
+        # parameter to its new place.
+        if item in sub:
+            logging.warning("Both deprecated parameter %s and new parameter "
+                            "%s are declared in configuration, deprecated "
+                            "parameter %s is ignored",
+                            param, replacement, param)
+        else:
+            sub[item] = value
+        del data[param]
+
+    def _move_deprecated(self, data):
+        """
+        Iterate over data dict to check for deprecated parameters and move them
+        to their replacements.
+        """
+        # Load generic options (ie. not architecture specific)
+        for param, value in data.copy().items():
+            # Skip architecture specific options
+            if param in self.get('arch'):
+                continue
+            self._move_deprecated_param(data, param, value)
+
+        # Load architecture specific options
+        for arch in self.get('arch'):
+            if arch in data and isinstance(data[arch], dict):
+                for param, value in data.copy()[arch].items():
+                    self._move_deprecated_param(data[arch], param, value)
+
     def update(self, data):
         """
         Update config content with data dict, checking data content respect
@@ -566,6 +699,9 @@ def update(self, data):
             self.set('arch', data['arch'])
             del data['arch']
 
+        # Look for deprecated parameters, and update dict with new parameters.
+        self._move_deprecated(data)
+
         # Load generic options (ie. not architecture specific)
         for key, value in data.items():
             # Skip architecture specific options
@@ -585,22 +721,45 @@ def update(self, data):
                     self.set(key, value, arch=arch)
 
     def _check(self):
-        """Checks for mandatory options."""
-        for key, value in self.SYNTAX.items():
+        """Checks for required options in main syntax recursively."""
+        self._check_syntax(self.SYNTAX, self.options)
+
+    def _check_syntax(self, syntax, options, param='__main__'):
+        """Checks for mandatory options regarding the provided syntax recursively."""
+        for key in syntax:
             if (
-                    value.get('required', False) and
-                    'default' not in value
+                    syntax[key].get('required', False) and
+                    'default' not in syntax[key]
                 ):
                 # Check key is in options or defined in all supported arch
                 # specific options.
                 if (
-                        key not in self.options and
+                        key not in options and
                         not all(
-                            arch in self.options and key in self.options[arch]
+                            arch in options and key in options[arch]
                             for arch in self.get('arch')
                         )
                     ):
-                    raise DeclError(f"'{key}' is not defined")
+                    if param == '__main__':
+                        raise DeclError(f"'{key}' is not defined")
+                    raise DeclError(
+                        f"Key {key} is required in dict parameter {param}"
+                    )
+            # If the parameter is a dict with a syntax, check the value.
+            if (
+                    syntax[key].get('check') == 'dict' and
+                    syntax[key].get('syntax') is not None and key in options
+                ):
+                self._check_syntax(syntax[key]['syntax'], options[key], key)
+            # If the parameter is a record with dict values and a syntax, check
+            # all values.
+            if (
+                    syntax[key].get('check') == 'record' and
+                    syntax[key].get('content') == 'dict' and
+                    syntax[key].get('syntax') is not None and key in options
+                ):
+                for value in options[key].values():
+                    self._check_syntax(syntax[key]['syntax'], value, key)
 
 
 class Staff():

lib/rift/Controller.py

@@ -682,7 +682,7 @@ def vm_build(vm, args, config):
             "Both --deploy and -o,--output options cannot be used together"
         )
     if args.deploy:
-        output = config.get('vm_image')
+        output = config.get('vm').get('image')
     else:
         output = args.output
     message(f"Building new vm image {output}")

lib/rift/Gerrit.py

@@ -80,15 +80,20 @@ def invalidate(self):
     def push(self, config, changeid, revid):
         """Send REST request to Gerrit server from config"""
         auth_methods = ('digest', 'basic')
-        realm = config.get('gerrit_realm')
-        server = config.get('gerrit_server')
-        username = config.get('gerrit_username')
-        password = config.get('gerrit_password')
-        auth_method = config.get('gerrit_auth_method', 'basic')
+
+        gerrit_config = config.get('gerrit')
+        if gerrit_config is None:
+            raise RiftError("Gerrit configuration is not defined")
+
+        realm = gerrit_config.get('realm')
+        server = gerrit_config.get('server')
+        username = gerrit_config.get('username')
+        password = gerrit_config.get('password')
+        auth_method = gerrit_config.get('auth_method', 'basic')
 
         if realm is None:
             raise RiftError("Gerrit realm is not defined")
-        if server is None and config.get('gerrit_url') is None:
+        if server is None and gerrit_config.get('url') is None:
             raise RiftError("Gerrit url is not defined")
         if username is None:
             raise RiftError("Gerrit username is not defined")
@@ -98,7 +103,7 @@ def push(self, config, changeid, revid):
             raise RiftError(f"Gerrit auth_method is not correct (supported {auth_methods})")
 
         # Set a default url if only gerrit_server was defined
-        url = config.get('gerrit_url', f"https://{server}")
+        url = gerrit_config.get('url', f"https://{server}")
 
         # FIXME: Don't check certificate
         ctx = ssl.create_default_context()