From 855801959a6d01d957647da383a531465adba95b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9mi=20Palancher?= <remi@rackslab.io>
Date: Wed, 16 Jul 2025 15:41:58 +0200
Subject: [PATCH 1/4] Config: check all required params recursively

Instead of checking for required in params in sub dictionnaries at load
time, check them recursively after everything is loaded.

This change notably fixes bug in case required parameter in a sub
dictionnary is not defined in a configuration file but defined in
another file and in merged options eventually.
---
 lib/rift/Config.py | 46 ++++++++++++++++++++++++++++++++--------------
 tests/Config.py    | 30 +++++++++++++++++++++++++++++-
 2 files changed, 61 insertions(+), 15 deletions(-)

diff --git a/lib/rift/Config.py b/lib/rift/Config.py
index 5fe02806..9e0e061c 100644
--- a/lib/rift/Config.py
+++ b/lib/rift/Config.py
@@ -522,8 +522,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 +533,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):
@@ -585,22 +580,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():
diff --git a/tests/Config.py b/tests/Config.py
index 6ee331d7..bed3f23e 100644
--- a/tests/Config.py
+++ b/tests/Config.py
@@ -501,7 +501,6 @@ def test_load_gpg_missing_keyring_or_key(self):
                     '^Key (key|keyring) is required in dict parameter gpg$'
                 ):
                 config.load(cfgfile.name)
-            self.assertEqual(config.get('gpg'), None)
 
     def test_load_gpg_unknown_key(self):
         """Load gpg parameters raise DeclError if unknown key"""
@@ -1058,6 +1057,35 @@ def test_load_dict_merged_syntax(self):
         self.assertEqual(param0['key1'], 'value2')
         self.assertEqual(param0['key2'], 1)
 
+    def test_load_dict_merged_syntax_missing_required(self):
+        """load() merges dict from multiple files with syntax and required param missing in one file"""
+        self._add_fake_params()
+        conf_files = [
+            make_temp_file(
+                textwrap.dedent(
+                    """
+                    param0:
+                      key1: value1
+                    """
+                )
+            ),
+            make_temp_file(
+                textwrap.dedent(
+                    """
+                    param0:
+                      key2: 1
+                    """
+                )
+            ),
+        ]
+        config = Config()
+        config.load([conf_file.name for conf_file in conf_files])
+        param0 = config.get('param0')
+        self.assertTrue('key1' in param0)
+        self.assertTrue('key2' in param0)
+        self.assertEqual(param0['key1'], 'value1')
+        self.assertEqual(param0['key2'], 1)
+
     def test_load_record(self):
         """Load record without content"""
         Config.SYNTAX.update({

From 57c0017c4da4aa9ca55cca0e14cff55183ecc060 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9mi=20Palancher?= <remi@rackslab.io>
Date: Fri, 15 Nov 2024 10:17:53 +0100
Subject: [PATCH 2/4] Config: add possibility to deprecate parameters

This is now possible to mark configuration parameters as deprecated, by
specifying the name of their replacement. Arbitrary levels of dict keys
can be specified in the name using dot separator (ex: hash1.hash2.key3).

A FutureWarning is emitted when deprecated parameter is loaded, to alert
user of the change and inform of the new parameter name.

When both the deprecated and replacement parameters are defined, the
deprecated parameter is ignored.

Unit tests are introduced to cover all cases of this feature.
---
 lib/rift/Config.py |  82 +++++++++++++++++
 tests/Config.py    | 214 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 295 insertions(+), 1 deletion(-)

diff --git a/lib/rift/Config.py b/lib/rift/Config.py
index 9e0e061c..c06bb08f 100644
--- a/lib/rift/Config.py
+++ b/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)
@@ -444,6 +450,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],
@@ -551,6 +563,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
@@ -561,6 +640,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
diff --git a/tests/Config.py b/tests/Config.py
index bed3f23e..547c7af4 100644
--- a/tests/Config.py
+++ b/tests/Config.py
@@ -17,7 +17,8 @@
                          _DEFAULT_QEMU_CMD, _DEFAULT_REPO_CMD, \
                          _DEFAULT_SHARED_FS_TYPE, _DEFAULT_VIRTIOFSD, \
                          _DEFAULT_SYNC_METHOD, _DEFAULT_SYNC_INCLUDE, \
-                         _DEFAULT_SYNC_EXCLUDE
+                         _DEFAULT_SYNC_EXCLUDE, \
+                         RiftDeprecatedConfWarning
 
 class ConfigTest(RiftTestCase):
 
@@ -1273,6 +1274,217 @@ def test_load_record_merged(self):
         self.assertEqual(record0['value2'], 20)
         self.assertEqual(record0['value3'], 3)
 
+    def test_deprecated_param(self):
+        """Load deprecated parameter"""
+        Config.SYNTAX.update({
+            'new_parameter': {},
+            'old_parameter': {
+                'deprecated': 'new_parameter',
+            },
+        })
+        cfgfile = make_temp_file(
+            textwrap.dedent(
+                """
+                old_parameter: test_value
+                """
+            )
+        )
+        config = Config()
+        with self.assertWarns(RiftDeprecatedConfWarning) as cm:
+            config.load(cfgfile.name)
+        self.assertEqual(
+            config.get('new_parameter'), 'test_value'
+        )
+        self.assertIsNone(config.get('old_parameter'))
+        self.assertEqual(
+            str(cm.warning),
+            "Configuration parameter old_parameter is deprecated, use "
+            "new_parameter instead"
+        )
+        # Check set() on deprecated parameter raise declaration error.
+        with self.assertRaisesRegex(
+            DeclError,
+            "^Parameter old_parameter is deprecated, use new_parameter instead$"
+        ):
+            config.set('old_parameter', 'another value')
+
+
+    def test_deprecated_param_with_arch(self):
+        """Load deprecated parameter with arch override"""
+        Config.SYNTAX.update({
+            'new_parameter_1': {},
+            'old_parameter_1': {
+                'deprecated': 'new_parameter_1',
+            },
+            'new_parameter_2': {},
+            'old_parameter_2': {
+                'deprecated': 'new_parameter_2',
+            },
+        })
+        cfgfile = make_temp_file(
+            textwrap.dedent(
+                """
+                arch:
+                - x86_64
+                - aarch64
+                old_parameter_1: generic_value
+                aarch64:
+                    old_parameter_1: aarch64_value
+                old_parameter_2: generic_value_$arch
+                """
+            )
+        )
+        config = Config()
+        with self.assertWarns(RiftDeprecatedConfWarning):
+            config.load(cfgfile.name)
+        self.assertEqual(config.get('new_parameter_1'), 'generic_value')
+        self.assertEqual(
+            config.get('new_parameter_1', arch='x86_64'), 'generic_value'
+        )
+        self.assertEqual(
+            config.get('new_parameter_1', arch='aarch64'), 'aarch64_value'
+        )
+        self.assertEqual(
+            config.get('new_parameter_2', arch='x86_64'), 'generic_value_x86_64'
+        )
+        self.assertEqual(
+            config.get('new_parameter_2', arch='aarch64'),
+            'generic_value_aarch64'
+        )
+        self.assertIsNone(config.get('old_parameter_1'))
+        self.assertIsNone(config.get('old_parameter_2'))
+
+    def test_deprecated_param_subdict(self):
+        """Load deprecated parameter moved in subdict"""
+        Config.SYNTAX.update({
+            'new_parameter': {
+                'check': 'dict',
+                'syntax': {
+                    'sub_dict1': {
+                        'check': 'dict',
+                        'syntax': {
+                            'new_key1': {}
+                        },
+                    },
+                },
+            },
+            'old_parameter': {
+                'deprecated': 'new_parameter.sub_dict1.new_key1',
+            },
+        })
+        cfgfile = make_temp_file(
+            textwrap.dedent(
+                """
+                old_parameter: test_value
+                """
+            )
+        )
+        config = Config()
+        with self.assertWarns(RiftDeprecatedConfWarning) as cm:
+            config.load(cfgfile.name)
+        self.assertEqual(
+            config.get('new_parameter').get('sub_dict1').get('new_key1'), 'test_value'
+        )
+        self.assertIsNone(config.get('old_parameter'))
+        self.assertEqual(
+            str(cm.warning),
+            "Configuration parameter old_parameter is deprecated, use "
+            "new_parameter > sub_dict1 > new_key1 instead"
+        )
+
+    def test_deprecated_param_invalid_type(self):
+        """Deprecated parameter with invalid type error"""
+        Config.SYNTAX.update({
+            'new_parameter': {
+                'check': 'digit',
+            },
+            'old_parameter': {
+                'deprecated': 'new_parameter',
+            },
+        })
+        cfgfile = make_temp_file(
+            textwrap.dedent(
+                """
+                old_parameter: test_value
+                """
+            )
+        )
+        config = Config()
+        # In this case, Config.set() should emit a declaration error on
+        # new_parameter. Also check warning is emited for old_parameter.
+        with self.assertWarns(RiftDeprecatedConfWarning) as aw:
+            with self.assertRaisesRegex(
+                DeclError,
+                "Bad data type str for 'new_parameter'"
+            ):
+                config.load(cfgfile.name)
+        self.assertEqual(
+            str(aw.warning),
+            "Configuration parameter old_parameter is deprecated, use "
+            "new_parameter instead"
+        )
+
+    def test_deprecated_param_unexisting_replacement(self):
+        """Deprecated parameter without replacement error"""
+        Config.SYNTAX.update({
+            'old_parameter': {
+                'deprecated': 'new_parameter',
+            },
+        })
+        cfgfile = make_temp_file(
+            textwrap.dedent(
+                """
+                old_parameter: test_value
+                """
+            )
+        )
+        config = Config()
+        # In this case, Config.set() should emit a declaration error.
+        with self.assertWarns(RiftDeprecatedConfWarning) as aw:
+            with self.assertRaisesRegex(
+                DeclError, "Unknown 'new_parameter' key"):
+                config.load(cfgfile.name)
+        self.assertEqual(
+            str(aw.warning),
+            "Configuration parameter old_parameter is deprecated, use "
+            "new_parameter instead"
+        )
+
+    def test_deprecated_param_conflict(self):
+        """Load deprecated parameter conflict with replacement parameter"""
+        Config.SYNTAX.update({
+            'new_parameter': {},
+            'old_parameter': {
+                'deprecated': 'new_parameter',
+            },
+        })
+        cfgfile = make_temp_file(
+            textwrap.dedent(
+                """
+                new_parameter: test_new_value
+                old_parameter: test_old_value
+                """
+            )
+        )
+        config = Config()
+        with self.assertWarns(RiftDeprecatedConfWarning) as aw:
+            with self.assertLogs(level='WARNING') as al:
+                config.load(cfgfile.name)
+        self.assertEqual(
+            config.get('new_parameter'), 'test_new_value'
+        )
+        self.assertIsNone(config.get('old_parameter'))
+        self.assertEqual(
+            str(aw.warning),
+            "Configuration parameter old_parameter is deprecated, use "
+            "new_parameter instead"
+        )
+        self.assertEqual(
+            al.output,
+            ['WARNING:root:Both deprecated parameter old_parameter and new '
+             'parameter new_parameter are declared in configuration, '
+             'deprecated parameter old_parameter is ignored']
+        )
 
 class ProjectConfigTest(RiftTestCase):
 

From 3aae0dd5ad2f58b19c205ab3e39f04e05d64ae31 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9mi=20Palancher?= <remi@rackslab.io>
Date: Fri, 15 Nov 2024 10:18:43 +0100
Subject: [PATCH 3/4] Config: deprecate vm_* with vm dict

Replace all vm_* parameters by the same parameters in vm dict (eg.
vm_image is becoming vm > image). All vm_* parameters are marked as
deprecated, ie. they are still supported but Rift emits warning visible
by end users to report the new parameter name.
---
 lib/rift/Config.py     |  91 +++++++++++++++++-------
 lib/rift/Controller.py |   2 +-
 lib/rift/VM.py         |  25 +++----
 template/project.conf  |  81 +++++++++++----------
 tests/Config.py        | 156 +++++++++++++++++++++++++++--------------
 tests/Controller.py    |  10 +--
 tests/TestUtils.py     |  23 ++++--
 tests/VM.py            |  19 +++--
 8 files changed, 257 insertions(+), 150 deletions(-)

diff --git a/lib/rift/Config.py b/lib/rift/Config.py
index c06bb08f..b5f1ff01 100644
--- a/lib/rift/Config.py
+++ b/lib/rift/Config.py
@@ -181,49 +181,88 @@ 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_realm': {},
         'gerrit_server': {},
diff --git a/lib/rift/Controller.py b/lib/rift/Controller.py
index 6a466fce..b6ebb433 100644
--- a/lib/rift/Controller.py
+++ b/lib/rift/Controller.py
@@ -677,7 +677,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}")
diff --git a/lib/rift/VM.py b/lib/rift/VM.py
index 3382bec8..107fb0a3 100644
--- a/lib/rift/VM.py
+++ b/lib/rift/VM.py
@@ -123,7 +123,8 @@ def __init__(self, config, arch, tmpmode=True, extra_repos=None):
         self.version = config.get('version', '0')
         self.arch = arch
 
-        self._image = config.get('vm_image', arch=arch)
+        vm_config = config.get('vm', arch=arch)
+        self._image = vm_config.get('image')
         self._project_dir = config.project_dir
 
         if extra_repos is None:
@@ -131,17 +132,17 @@ def __init__(self, config, arch, tmpmode=True, extra_repos=None):
 
         self._repos = ProjectArchRepositories(config, arch).all + extra_repos
 
-        self.address = config.get('vm_address')
-        self.port = self.default_port(config.get('vm_port_range'))
-        self.cpus = config.get('vm_cpus', 1)
-        self.memory = config.get('vm_memory')
+        self.address = vm_config.get('address')
+        self.port = self.default_port(vm_config.get('port_range'))
+        self.cpus = vm_config.get('cpus', 1)
+        self.memory = vm_config.get('memory')
         self.qemu = config.get('qemu', arch=arch)
 
         # default emulated cpu architecture
         if self.arch == 'aarch64':
-            self.cpu_type = config.get('vm_cpu', 'cortex-a72')
+            self.cpu_type = vm_config.get('cpu', 'cortex-a72')
         else:
-            self.cpu_type = config.get('vm_cpu', 'host')
+            self.cpu_type = vm_config.get('cpu', 'host')
 
         # Specific aarch64 options
         self.arch_efi_bios = config.get('arch_efi_bios', ARCH_EFI_BIOS)
@@ -156,21 +157,21 @@ def __init__(self, config, arch, tmpmode=True, extra_repos=None):
 
 
         self.tmpmode = tmpmode
-        self.copymode = config.get('vm_image_copy')
+        self.copymode = vm_config.get('image_copy')
         self._vm = None
         self._helpers = []
         self._tmpimg = None
         self.consolesock = f"/tmp/rift-vm-console-{self.vmid}.sock"
         self.proxy = config.get('proxy')
         self.no_proxy = config.get('no_proxy')
-        self.additional_rpms = config.get('vm_additional_rpms')
+        self.additional_rpms = vm_config.get('additional_rpms')
         self.cloud_init_tpl = config.project_path(
-            config.get('vm_cloud_init_tpl')
+            vm_config.get('cloud_init_tpl')
         )
         self.build_post_script = config.project_path(
-            config.get('vm_build_post_script')
+            vm_config.get('build_post_script')
         )
-        self.images_cache = config.get('vm_images_cache')
+        self.images_cache = vm_config.get('images_cache')
 
     @property
     def vmid(self):
diff --git a/template/project.conf b/template/project.conf
index 75b6f876..8dead750 100644
--- a/template/project.conf
+++ b/template/project.conf
@@ -15,16 +15,6 @@ annex: /somewhere/
 #
 # no_proxy: localhost,.intranet.company.ltd
 
-# VM options
-vm_image: images/default.qcow
-# full copy of image
-vm_image_copy: False
-
-# TCP port range Rift can select for SSH server in the VMs
-# vm_port_range:
-#   min: 10000
-#   max: 15000
-
 # Build architectures
 arch:
 - x86_64
@@ -39,37 +29,51 @@ arch:
 #rpm_macros:
 #  kernel_version: 1.2.4.4
 
-# Optional VM settings
-#
-# Path to directory where downloaded cloud images are stored locally. This
-# directory serves as a cache: when images are already present in this directory
-# (ie. same filename), download is skipped. If this parameter is not defined,
-# images are downloaded in a temporary directory.
-#
-# vm_images_cache: /path/to/images/cache
-#
-# List of paths to RPM packages that are copied into the VM before execution of
-# build post script. This script is then responsible of their installation in VM
-# image.
-#
-# vm_additional_rpms:
-# - /path/to/first-rpm-1.el8.x86_64.rpm
-# - /path/to/second-rpm-1.el8.x86_64.rpm
-#
-# Path to alternative cloud-init template file. By default, Rift uses
-# cloud-init.tpl file located in project top folder.
-#
-# vm_cloud_init_tpl: /path/to/cloud-init.tpl
-#
-# Path to alternative VM build post script. By default, Rift executes
-# build-post.sh script located in project top folder.
-#
-# vm_build_post_script: /path/to/build-post.sh
+# VM options
+vm:
+  image: images/default.qcow
+
+  # Optional VM settings
+  #
+  # Make a full copy of the image before using it.
+  # image_copy: False
+  #
+  # TCP port range Rift can select for SSH server for its VMs.
+  # port_range:
+  #   min: 10000
+  #   max: 15000
+  #
+  # Path to directory where downloaded cloud images are stored locally. This
+  # directory serves as a cache: when images are already present in this directory
+  # (ie. same filename), download is skipped. If this parameter is not defined,
+  # images are downloaded in a temporary directory.
+  #
+  # images_cache: /path/to/images/cache
+  #
+  # List of paths to RPM packages that are copied into the VM before execution of
+  # build post script. This script is then responsible of their installation in VM
+  # image.
+  #
+  # additional_rpms:
+  # - /path/to/first-rpm-1.el8.x86_64.rpm
+  # - /path/to/second-rpm-1.el8.x86_64.rpm
+  #
+  # Path to alternative cloud-init template file. By default, Rift uses
+  # cloud-init.tpl file located in project top folder.
+  #
+  # cloud_init_tpl: /path/to/cloud-init.tpl
+  #
+  # Path to alternative VM build post script. By default, Rift executes
+  # build-post.sh script located in project top folder.
+  #
+  # Emulated CPU model for the VM. The default value is 'cortex-a72' for aarch64
+  # architecture or 'host' for other architectures.
+  # cpu: "cortex-a72"
+  # build_post_script: /path/to/build-post.sh
 
 # Dedicated options for aarch64 builds
 #arch: "aarch64"
 #arch_efi_bios: "/ccc/home/cont001/ocre/cedeyna/Ocean/rift/vendor/QEMU_EFI.fd"
-#vm_cpu: "cortex-a72"
 
 # Example GPG settings for package cryptographic signing
 #gpg:
@@ -83,7 +87,8 @@ arch:
 # It is possible to declare architecture specific options with a mapping under
 # the key named after this architectur.
 # x86_64:
-#   vm_image: images/image-x86_64.qcow2
+#   vm:
+#     image: images/image-x86_64.qcow2
 
 # External repositories
 #
diff --git a/tests/Config.py b/tests/Config.py
index 547c7af4..26b5acb4 100644
--- a/tests/Config.py
+++ b/tests/Config.py
@@ -30,12 +30,12 @@ def test_get(self):
         self.assertEqual(config.get('packages_dir'), _DEFAULT_PKG_DIR)
         self.assertEqual(config.get('staff_file'), _DEFAULT_STAFF_FILE)
         self.assertEqual(config.get('modules_file'), _DEFAULT_MODULES_FILE)
-        self.assertEqual(config.get('vm_cpus'), _DEFAULT_VM_CPUS)
-        self.assertEqual(config.get('vm_address'), _DEFAULT_VM_ADDRESS)
+        self.assertEqual(config.get('vm').get('cpus'), _DEFAULT_VM_CPUS)
+        self.assertEqual(config.get('vm').get('address'), _DEFAULT_VM_ADDRESS)
         self.assertEqual(config.get('shared_fs_type'), _DEFAULT_SHARED_FS_TYPE)
         self.assertEqual(config.get('virtiofsd'), _DEFAULT_VIRTIOFSD)
         self.assertEqual(
-            config.get('vm_port_range'),
+            config.get('vm').get('port_range'),
             {
                 'min': _DEFAULT_VM_PORT_RANGE_MIN,
                 'max': _DEFAULT_VM_PORT_RANGE_MAX
@@ -57,12 +57,21 @@ def test_get_set(self):
         """simple set() and get()"""
         config = Config()
         # set an 'int'
-        config.set('vm_cpus', 42)
-        self.assertEqual(config.get('vm_cpus'), 42)
+        config.set('vm', {'image': '/path/to/image', 'cpus': 42})
+        self.assertEqual(config.get('vm').get('cpus'), 42)
 
         # set a 'dict'
-        config.set('vm_port_range', {'min': 5000, 'max': 6000})
-        self.assertEqual(config.get('vm_port_range'), {'min': 5000, 'max': 6000})
+        config.set(
+            'vm',
+            {
+                'image': '/path/to/image',
+                'port_range': {'min': 5000, 'max': 6000}
+            }
+        )
+        self.assertEqual(
+            config.get('vm').get('port_range'),
+            {'min': 5000, 'max': 6000}
+        )
 
         # set a 'record'
         config.set('repos', {'os': {'url': 'http://myserver/pub'}})
@@ -78,13 +87,13 @@ def test_get_set(self):
 
     def test_set_bad_type(self):
         """set() using wrong type raises an error"""
-        self.assert_except(DeclError, "Bad data type str for 'vm_cpus'",
-                           Config().set, 'vm_cpus', 'a string')
+        self.assert_except(DeclError, "Bad data type str for 'cpus'",
+                Config().set, 'vm', {'image': '/path/to/image', 'cpus': 'a string'})
         self.assert_except(DeclError, "Bad data type str for 'repos'",
                            Config().set, 'repos', 'a string')
         # Default check is 'string'
-        self.assert_except(DeclError, "Bad data type int for 'vm_image'",
-                           Config().set, 'vm_image', 42)
+        self.assert_except(DeclError, "Bad data type int for 'image'",
+                Config().set, 'vm', {'image': 42})
         self.assert_except(DeclError,
                            "Bad data type str for 'arch'",
                            Config().set, 'arch', 'x86_64')
@@ -105,13 +114,13 @@ def test_get_arch_placeholder(self):
         # Declare supported architectures
         config.set('arch', ['x86_64', 'aarch64'])
         # $arch placeholder replacement with string
-        config.set('vm_image', '/path/to/image-$arch.qcow2')
+        config.set('vm', {'image': '/path/to/image-$arch.qcow2'})
         self.assertEqual(
-            config.get('vm_image', arch='x86_64'),
+            config.get('vm', arch='x86_64').get('image'),
             '/path/to/image-x86_64.qcow2'
         )
         self.assertEqual(
-            config.get('vm_image', arch='aarch64'),
+            config.get('vm', arch='aarch64').get('image'),
             '/path/to/image-aarch64.qcow2'
         )
         # $arch placeholder replacement with dict
@@ -162,14 +171,14 @@ def test_get_arch_specific_override(self):
         # Declare supported architectures
         config.set('arch', ['x86_64', 'aarch64'])
         # Override with arch specific value
-        config.set('vm_image', '/path/to/image-$arch.qcow2')
-        config.set('vm_image', '/path/to/other-image.qcow2', arch='x86_64')
+        config.set('vm', {'image': '/path/to/image-$arch.qcow2'})
+        config.set('vm', {'image': '/path/to/other-image.qcow2'}, arch='x86_64')
         self.assertEqual(
-            config.get('vm_image', arch='aarch64'),
+            config.get('vm', arch='aarch64').get('image'),
             '/path/to/image-aarch64.qcow2'
         )
         self.assertEqual(
-            config.get('vm_image', arch='x86_64'),
+            config.get('vm', arch='x86_64').get('image'),
             '/path/to/other-image.qcow2'
         )
 
@@ -183,7 +192,7 @@ def test_get_unsupported_arch(self):
             "^Unable to get configuration option for unsupported architecture "
             "'fail'$"
         ):
-            config.get('vm_image', arch='fail')
+            config.get('vm', arch='fail')
 
     def test_set_unsupported_arch(self):
         """set() with unsupported arch"""
@@ -194,7 +203,7 @@ def test_set_unsupported_arch(self):
             "^Unable to set configuration option for unsupported architecture "
             "'fail'$"
         ):
-            config.set('vm_image', '/path/to/image.qcow2', arch='fail')
+            config.set('vm', {'image': '/path/to/image.qcow2'}, arch='fail')
 
 
     def test_load(self):
@@ -203,7 +212,7 @@ def test_load(self):
         self.assert_except(DeclError, "'annex' is not defined",
                            Config().load, emptyfile.name)
 
-        cfgfile = make_temp_file("annex: /a/dir\nvm_image: /a/image.img")
+        cfgfile = make_temp_file("annex: /a/dir\nvm:\n  image: /a/image.img")
         config = Config()
         # Simple filename
         config.load(cfgfile.name)
@@ -223,14 +232,16 @@ def test_load_multiple_files(self):
                 textwrap.dedent(
                     """
                     annex: /a/dir
-                    vm_image: /a/image.img
+                    vm:
+                      image: /a/image.img
                     """
                 )
             ),
             make_temp_file(
                 textwrap.dedent(
                     """
-                    vm_image: /b/image.img
+                    vm:
+                      image: /b/image.img
                     arch:
                     - x86_64
                     - aarch64
@@ -243,7 +254,7 @@ def test_load_multiple_files(self):
         # Value from 1st file should be loaded
         self.assertEqual(config.get('annex'), '/a/dir')
         # Value from 2nd file should override value from 1st file
-        self.assertEqual(config.get('vm_image'), '/b/image.img')
+        self.assertEqual(config.get('vm').get('image'), '/b/image.img')
         # Value from 2nd file should be loaded
         self.assertEqual(config.get('arch'), ['x86_64', 'aarch64'])
 
@@ -253,22 +264,25 @@ def test_load_arch_specific(self):
             textwrap.dedent(
                 """
                 annex: /a/dir
-                vm_image: /a/image.img
+                vm:
+                  image: /a/image.img
                 arch:
                 - x86_64
                 - aarch64
                 x86_64:
-                    vm_image: /b/image.img
+                  vm:
+                    image: /b/image.img
                 aarch64:
-                    vm_image: /c/image.img
+                  vm:
+                    image: /c/image.img
                 """
             )
         )
         config = Config()
         config.load(cfgfile.name)
-        self.assertEqual(config.get('vm_image'), '/a/image.img')
-        self.assertEqual(config.get('vm_image', arch='x86_64'), '/b/image.img')
-        self.assertEqual(config.get('vm_image', arch='aarch64'), '/c/image.img')
+        self.assertEqual(config.get('vm').get('image'), '/a/image.img')
+        self.assertEqual(config.get('vm', arch='x86_64').get('image'), '/b/image.img')
+        self.assertEqual(config.get('vm', arch='aarch64').get('image'), '/c/image.img')
 
     def test_load_arch_specific_invalid_mapping(self):
         """load() fail with not mapping architecture specific options"""
@@ -276,7 +290,8 @@ def test_load_arch_specific_invalid_mapping(self):
             textwrap.dedent(
                 """
                 annex: /a/dir
-                vm_image: /a/image.img
+                vm:
+                  image: /a/image.img
                 x86_64: fail
                 """
             )
@@ -294,7 +309,8 @@ def test_load_arch_specific_invalid_key(self):
             textwrap.dedent(
                 """
                 annex: /a/dir
-                vm_image: /a/image.img
+                vm:
+                  image: /a/image.img
                 x86_64:
                     fail: value
                 """
@@ -321,7 +337,8 @@ def test_load_missing_required_key(self):
             - x86_64
             - aarch64
             x86_64:
-              vm_image: /a/image.img
+              vm:
+                image: /a/image.img
             """
         }
         for content in contents:
@@ -329,7 +346,7 @@ def test_load_missing_required_key(self):
             config = Config()
             with self.assertRaisesRegex(
                 DeclError,
-                "^'vm_image' is not defined$",
+                "^'vm' is not defined$",
             ):
                 config.load(cfgfile.name)
 
@@ -343,9 +360,11 @@ def test_load_required_key_in_archs_ok(self):
                 - x86_64
                 - aarch64
                 x86_64:
-                    vm_image: /b/image.img
+                  vm:
+                    image: /b/image.img
                 aarch64:
-                    vm_image: /c/image.img
+                  vm:
+                    image: /c/image.img
                 """
             )
         )
@@ -375,7 +394,8 @@ def test_load_repos_merged(self):
                 textwrap.dedent(
                     """
                     annex: /a/dir
-                    vm_image: /a/image.img
+                    vm:
+                      image: /a/image.img
                     repos:
                       os:
                         url: https://os/url/file1
@@ -414,35 +434,37 @@ def test_load_port_partial_port_range(self):
             textwrap.dedent(
                 """
                 annex: /a/dir
-                vm_image: /a/image.img
-                vm_port_range:
-                  min: 2000
+                vm:
+                  image: /a/image.img
+                  port_range:
+                    min: 2000
                 """
             )
         )
         config = Config()
         config.load(cfgfile.name)
-        self.assertEqual(config.get('vm_port_range').get('min'), 2000)
+        self.assertEqual(config.get('vm').get('port_range').get('min'), 2000)
         self.assertEqual(
-            config.get('vm_port_range').get('max'),
+            config.get('vm').get('port_range').get('max'),
             _DEFAULT_VM_PORT_RANGE_MAX
         )
         cfgfile = make_temp_file(
             textwrap.dedent(
                 """
                 annex: /a/dir
-                vm_image: /a/image.img
-                vm_port_range:
-                  max: 30000
+                vm:
+                  image: /a/image.img
+                  port_range:
+                    max: 30000
                 """
             )
         )
         config.load(cfgfile.name)
         self.assertEqual(
-            config.get('vm_port_range').get('min'),
+            config.get('vm').get('port_range').get('min'),
             _DEFAULT_VM_PORT_RANGE_MIN
         )
-        self.assertEqual(config.get('vm_port_range').get('max'), 30000)
+        self.assertEqual(config.get('vm').get('port_range').get('max'), 30000)
 
     def test_load_gpg(self):
         """Load gpg parameters"""
@@ -451,7 +473,8 @@ def test_load_gpg(self):
             textwrap.dedent(
                 """
                 annex: /a/dir
-                vm_image: /a/image.img
+                vm:
+                  image: /a/image.img
                 gpg:
                   keyring: /path/to/keyring
                   key: rift
@@ -469,7 +492,8 @@ def test_load_gpg(self):
             textwrap.dedent(
                 """
                 annex: /a/dir
-                vm_image: /a/image.img
+                vm:
+                  image: /a/image.img
                 gpg:
                   keyring: /path/to/keyring
                   key: rift
@@ -491,7 +515,8 @@ def test_load_gpg_missing_keyring_or_key(self):
                 textwrap.dedent(
                     f"""
                     annex: /a/dir
-                    vm_image: /a/image.img
+                    vm:
+                      image: /a/image.img
                     gpg: {gpg_config}
                     """
                 )
@@ -509,7 +534,8 @@ def test_load_gpg_unknown_key(self):
             textwrap.dedent(
                 """
                 annex: /a/dir
-                vm_image: /a/image.img
+                vm:
+                  image: /a/image.img
                 gpg:
                   epic: fail
                   keyring: /path/to/keyring
@@ -528,7 +554,8 @@ def test_load_sync(self):
             textwrap.dedent(
                 """
                 annex: /a/dir
-                vm_image: /a/image.img
+                vm:
+                  image: /a/image.img
                 sync_output: /sync/output
                 repos:
                   repo1:
@@ -591,7 +618,8 @@ def test_load_sync_repo_missing_source(self):
             textwrap.dedent(
                 """
                 annex: /a/dir
-                vm_image: /a/image.img
+                vm:
+                  image: /a/image.img
                 repos:
                   repo1:
                     sync: {}
@@ -612,7 +640,8 @@ def test_load_sync_repo_invalid_method(self):
             textwrap.dedent(
                 """
                 annex: /a/dir
-                vm_image: /a/image.img
+                vm:
+                  image: /a/image.img
                 repos:
                   repo1:
                     sync:
@@ -629,6 +658,25 @@ def test_load_sync_repo_invalid_method(self):
         ):
             config.load(cfgfile.name)
 
+    def test_load_deprecated_vm_parameters(self):
+        """load() deprecated vm_* parameters."""
+        cfgfile = make_temp_file(
+            textwrap.dedent(
+                """
+                annex: /a/dir
+                vm_image: /my/custom/image.img
+                vm_cpus: 42
+                vm_memory: 1234
+                """
+            )
+        )
+        config = Config()
+        with self.assertWarns(RiftDeprecatedConfWarning):
+            config.load(cfgfile.name)
+        self.assertEqual(config.get('vm').get('image'), '/my/custom/image.img')
+        self.assertEqual(config.get('vm').get('cpus'), 42)
+        self.assertEqual(config.get('vm').get('memory'), 1234)
+
 
 class ConfigTestSyntax(RiftTestCase):
     """Test Config with modified syntax."""
diff --git a/tests/Controller.py b/tests/Controller.py
index 562bcba1..ac952b68 100644
--- a/tests/Controller.py
+++ b/tests/Controller.py
@@ -601,17 +601,17 @@ def test_action_vm_build(self, mock_vm_class):
         mock_vm_class.assert_called()
 
         mock_vm_objects.build.assert_called_once_with(
-            'http://image', False, False, self.config.get('vm_image')
+            'http://image', False, False, self.config.get('vm').get('image')
         )
         mock_vm_objects.build.reset_mock()
         main(['vm', 'build', 'http://image', '--deploy', '--force'])
         mock_vm_objects.build.assert_called_once_with(
-            'http://image', True, False, self.config.get('vm_image')
+            'http://image', True, False, self.config.get('vm').get('image')
         )
         mock_vm_objects.build.reset_mock()
         main(['vm', 'build', 'http://image', '--deploy', '--keep'])
         mock_vm_objects.build.assert_called_once_with(
-            'http://image', False, True, self.config.get('vm_image')
+            'http://image', False, True, self.config.get('vm').get('image')
         )
         mock_vm_objects.build.reset_mock()
         main(
@@ -645,12 +645,12 @@ def test_vm_build_and_validate(self):
         self.skipTest("Too much instability")
         if not os.path.exists("/usr/bin/qemu-img"):
             self.skipTest("qemu-img is not available")
-        self.config.options['vm_images_cache'] = GLOBAL_CACHE
+        self.config.options['vm']['images_cache'] = GLOBAL_CACHE
         # Reduce memory size from default 8GB to 2GB because it is sufficient to
         # run this VM and it largely reduces storage required by virtiofs memory
         # backend file which is the same size as the VM memory, thus reducing
         # the risk to fill up small partitions when running the tests.
-        self.config.options['vm_memory'] = 2048
+        self.config.options['vm']['memory'] = 2048
         self.config.options['proxy'] = PROXY
         self.config.options['repos'] = {
             'os': {
diff --git a/tests/TestUtils.py b/tests/TestUtils.py
index fb1d7fb5..e98001f3 100644
--- a/tests/TestUtils.py
+++ b/tests/TestUtils.py
@@ -162,7 +162,8 @@ def setUp(self):
         self.projectconf = os.path.join(self.projdir, Config._DEFAULT_FILES[0])
         with open(self.projectconf, "w") as conf:
             conf.write("annex:           %s\n" % self.annexdir)
-            conf.write("vm_image:        test.img\n")
+            conf.write("vm:\n")
+            conf.write("  image:         test.img\n")
             conf.write("repos:           {}\n")
         os.chdir(self.projdir)
         # Dict of created packages
@@ -198,9 +199,13 @@ def tearDown(self):
             os.rmdir(pkgdir)
         # Remove potentially generated files for VM related tests
         for path in [
-            self.config.project_path(self.config.get('vm_cloud_init_tpl')),
-            self.config.project_path(self.config.get('vm_build_post_script')),
-            self.config.project_path(self.config.get('vm_image')),
+            self.config.project_path(
+                self.config.get('vm').get('cloud_init_tpl')
+            ),
+            self.config.project_path(
+                self.config.get('vm').get('build_post_script')
+            ),
+            self.config.project_path(self.config.get('vm').get('image')),
         ]:
             if os.path.exists(path):
                 os.unlink(path)
@@ -318,7 +323,9 @@ def copy_cloud_init_tpl(self):
                 'template',
                 'cloud-init.tpl',
             ),
-            self.config.project_path(self.config.get('vm_cloud_init_tpl')),
+            self.config.project_path(
+                self.config.get('vm').get('cloud_init_tpl')
+            ),
         )
 
     def copy_build_post_script(self):
@@ -330,13 +337,15 @@ def copy_build_post_script(self):
                 'template',
                 'build-post.sh',
             ),
-            self.config.project_path(self.config.get('vm_build_post_script')),
+            self.config.project_path(
+                self.config.get('vm').get('build_post_script')
+            ),
         )
 
     def ensure_vm_images_cache_dir(self):
         """Ensure VM images cache directory exists."""
         cache_dir =  self.config.project_path(
-            self.config.get('vm_images_cache')
+            self.config.get('vm').get('images_cache')
         )
         if not os.path.exists(cache_dir):
             os.mkdir(cache_dir)
diff --git a/tests/VM.py b/tests/VM.py
index 63134f3f..2179436f 100644
--- a/tests/VM.py
+++ b/tests/VM.py
@@ -79,13 +79,18 @@ def test_init(self):
 
         # custom
         self.config.set('arch', ['aarch64'])
-        self.config.set('vm_cpu', 'custom')
-        self.config.set('vm_cpus', 32)
-        self.config.set('vm_memory', vm_custom_memory)
+        self.config.set(
+            'vm',
+            {
+                'cpu': 'custom',
+                'cpus': 32,
+                'memory': vm_custom_memory,
+                'image_copy': 1,
+                'address': '192.168.0.5',
+                'image': '/my_image',
+            }
+        )
         self.config.set('arch_efi_bios', '/my_bios')
-        self.config.set('vm_image_copy', 1)
-        self.config.set('vm_address', '192.168.0.5')
-        self.config.set('vm_image', '/my_image')
         self.config.set('qemu', '/my_custom_qemu')
 
         vm = VM(self.config, 'aarch64')
@@ -340,7 +345,7 @@ def setUp(self):
         super().setUp()
         # Override some configuration parameters defined in dummy config from
         # RiftProjectTestCase.
-        self.config.options['vm_images_cache'] = GLOBAL_CACHE
+        self.config.options['vm']['images_cache'] = GLOBAL_CACHE
         self.config.options['proxy'] = PROXY
         self.wrong_url = 'https://127.0.0.1/fail'
         self.valid_url = VALID_IMAGE_URL['x86_64']

From eb7c85ac82a7729881e354e78003318edb63714b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9mi=20Palancher?= <remi@rackslab.io>
Date: Mon, 18 Nov 2024 15:39:08 +0100
Subject: [PATCH 4/4] Config: deprecate gerrit_* with gerrit dict

Replace all gerrit_* parameters by the same parameters in gerrit dict
(eg. gerrit_realm is becoming gerrit > realm). All gerrit_* parameters
are marked as deprecated, ie. they are still supported but Rift emits
warning visible by end users to report the new parameter name.

Also add unit tests to cover Gerrit module.
---
 lib/rift/Config.py | 30 ++++++++++++++++++++-----
 lib/rift/Gerrit.py | 19 ++++++++++------
 tests/Config.py    | 21 +++++++++++++++++
 tests/Gerrit.py    | 56 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 114 insertions(+), 12 deletions(-)
 create mode 100644 tests/Gerrit.py

diff --git a/lib/rift/Config.py b/lib/rift/Config.py
index b5f1ff01..523a2822 100644
--- a/lib/rift/Config.py
+++ b/lib/rift/Config.py
@@ -264,11 +264,31 @@ class Config():
         'vm_build_post_script': {
             'deprecated': 'vm.build_post_script'
         },
-        'gerrit_realm': {},
-        'gerrit_server': {},
-        'gerrit_url': {},
-        'gerrit_username': {},
-        'gerrit_password': {},
+        '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'
+        },
         'rpm_macros': {
             'check':    'dict',
         },
diff --git a/lib/rift/Gerrit.py b/lib/rift/Gerrit.py
index 3c34da16..3c64ac00 100644
--- a/lib/rift/Gerrit.py
+++ b/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()
diff --git a/tests/Config.py b/tests/Config.py
index 26b5acb4..ef2335c4 100644
--- a/tests/Config.py
+++ b/tests/Config.py
@@ -677,6 +677,27 @@ def test_load_deprecated_vm_parameters(self):
         self.assertEqual(config.get('vm').get('cpus'), 42)
         self.assertEqual(config.get('vm').get('memory'), 1234)
 
+    def test_load_deprecated_gerrit_parameters(self):
+        """load() deprecated gerrit_* parameters."""
+        cfgfile = make_temp_file(
+            textwrap.dedent(
+                """
+                annex: /a/dir
+                vm:
+                  image: /a/image.img
+                gerrit_realm: Rift
+                gerrit_url: https://localhost
+                gerrit_username: rift
+                """
+            )
+        )
+        config = Config()
+        with self.assertWarns(RiftDeprecatedConfWarning):
+            config.load(cfgfile.name)
+        self.assertEqual(config.get('gerrit').get('realm'), 'Rift')
+        self.assertEqual(config.get('gerrit').get('url'), 'https://localhost')
+        self.assertEqual(config.get('gerrit').get('username'), 'rift')
+
 
 class ConfigTestSyntax(RiftTestCase):
     """Test Config with modified syntax."""
diff --git a/tests/Gerrit.py b/tests/Gerrit.py
new file mode 100644
index 00000000..992d807d
--- /dev/null
+++ b/tests/Gerrit.py
@@ -0,0 +1,56 @@
+#
+# Copyright (C) 2024 CEA
+#
+from unittest import mock
+
+from TestUtils import RiftTestCase
+from rift.Config import Config
+from rift.Gerrit import Review
+from rift import RiftError
+
+class GerritTest(RiftTestCase):
+    def setUp(self):
+        self.config = Config()
+        self.config.set(
+            'gerrit',
+            {
+                'realm': 'Rift',
+                'server': 'localhost',
+                'username': 'rift',
+                'password': 'SECR3T',
+            }
+        )
+        self.review = Review()
+        self.review.add_comment('/path/to/file', 42, 'E', 'test error message')
+
+    def test_invalidate(self):
+        """ Test Review.invalidate() method"""
+        self.assertEqual(self.review.validated, True)
+        self.review.invalidate()
+        self.assertEqual(self.review.validated, False)
+
+    @mock.patch("rift.Gerrit.urllib.urlopen")
+    def test_push(self, mock_urlopen):
+        """ Test Review push """
+        self.review.push(self.config, 4242, 42)
+        # Check push successfully send HTTP request with urllib.urlopen() and
+        # reads its result.
+        mock_urlopen.assert_called_once()
+        mock_urlopen.return_value.read.assert_called_once()
+
+    def test_push_no_config(self):
+        """ Test Review push w/o gerrit config error """
+        del self.config.options['gerrit']
+        with self.assertRaisesRegex(RiftError, "Gerrit configuration is not defined"):
+            self.review.push(self.config, 4242, 42)
+
+    def test_push_missing_conf_param(self):
+        """ Test Review push with missing parameter error """
+        gerrit_conf = self.config.get('gerrit')
+        for parameter, value in gerrit_conf.copy().items():
+            # temporary remove parameter
+            del gerrit_conf[parameter]
+            with self.assertRaisesRegex(RiftError, "Gerrit .* is not defined"):
+                self.review.push(self.config, 4242, 42)
+            # restore value
+            gerrit_conf[parameter] = value