From 3c66fc7cbedb3843c84a4284c858f55751548d5f Mon Sep 17 00:00:00 2001 From: Enrique Vallespi Gil Date: Fri, 3 Jul 2026 12:55:04 +0200 Subject: [PATCH] [reproducer] Override when value dict is {} Currently if override value is a dict {} this is not overriding base value no matter which value it has. We're modifying this to actual override, so we can disable hooks but also we can override network configurations as: cltplane: {} Co-authored-by: Cursor Signed-off-by: Enrique Vallespi Gil --- roles/reproducer/files/merge_yaml_override.py | 5 +- .../unit/modules/test_merge_yaml_override.py | 193 ++++++++++++++++++ 2 files changed, 197 insertions(+), 1 deletion(-) create mode 100644 tests/unit/modules/test_merge_yaml_override.py diff --git a/roles/reproducer/files/merge_yaml_override.py b/roles/reproducer/files/merge_yaml_override.py index 8af81a437..5b8b05c58 100644 --- a/roles/reproducer/files/merge_yaml_override.py +++ b/roles/reproducer/files/merge_yaml_override.py @@ -56,11 +56,14 @@ def _str_representer(dumper, data): def deep_merge(base: object, override: object) -> object: """Take two parsed values; - return merge where both are dicts (recursive), else override wins.""" + return merge where both are dicts (recursive), else override wins. + Empty dict in override replaces base value entirely.""" if base is None: return override if override is None: return base + if isinstance(override, dict) and len(override) == 0: + return {} if isinstance(base, dict) and isinstance(override, dict): out = dict(base) for key, val in override.items(): diff --git a/tests/unit/modules/test_merge_yaml_override.py b/tests/unit/modules/test_merge_yaml_override.py new file mode 100644 index 000000000..da56fea0b --- /dev/null +++ b/tests/unit/modules/test_merge_yaml_override.py @@ -0,0 +1,193 @@ +# Copyright Red Hat, Inc. +# All Rights Reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +"""Unit tests for merge_yaml_override.py deep_merge function.""" + +import sys +import unittest +from pathlib import Path + +# Import the deep_merge function from the reproducer role +sys.path.insert(0, str(Path(__file__).parents[3] / "roles" / "reproducer" / "files")) +from merge_yaml_override import deep_merge + + +class TestDeepMerge(unittest.TestCase): + """Test deep_merge function behavior.""" + + def test_empty_dict_overrides_non_empty_dict(self): + """Empty dict in override should replace base dict entirely.""" + base = {"hook": {"type": "playbook", "source": "test.yml"}} + override = {"hook": {}} + result = deep_merge(base, override) + self.assertEqual(result, {"hook": {}}) + + def test_nested_empty_dict_override(self): + """Empty dict at nested level should override base value.""" + base = {"outer": {"inner": {"key": "value"}}} + override = {"outer": {"inner": {}}} + result = deep_merge(base, override) + self.assertEqual(result, {"outer": {"inner": {}}}) + + def test_normal_dict_merge(self): + """Non-empty dicts should merge recursively, combining keys.""" + base = {"a": {"x": 1, "y": 2}} + override = {"a": {"y": 3, "z": 4}} + result = deep_merge(base, override) + expected = {"a": {"x": 1, "y": 3, "z": 4}} + self.assertEqual(result, expected) + + def test_scalar_override(self): + """Scalar values should override base completely.""" + base = {"key": "old"} + override = {"key": "new"} + result = deep_merge(base, override) + self.assertEqual(result, {"key": "new"}) + + def test_list_override(self): + """Lists should override entirely, not merge elements.""" + base = {"items": [1, 2, 3]} + override = {"items": [4, 5]} + result = deep_merge(base, override) + self.assertEqual(result, {"items": [4, 5]}) + + def test_none_base(self): + """When base is None, override should win.""" + base = None + override = {"key": "value"} + result = deep_merge(base, override) + self.assertEqual(result, {"key": "value"}) + + def test_none_override(self): + """When override is None, base should win.""" + base = {"key": "value"} + override = None + result = deep_merge(base, override) + self.assertEqual(result, {"key": "value"}) + + def test_both_none(self): + """When both are None, None should be returned.""" + base = None + override = None + result = deep_merge(base, override) + self.assertIsNone(result) + + def test_manila_hook_disable_scenario(self): + """Real-world scenario: disabling Manila hook in day2 scenarios. + + This is the regression test for the PreMetal bug where empty dict + failed to override the hook definition from bootstrap phase. + """ + # Bootstrap phase - hook defined in 05-hooks.yaml + base = { + "post_admin_setup_05_create_manila_resources": { + "type": "playbook", + "source": "manila_create_default_resources.yml", + } + } + # Testing phase - day2/scenario.yaml tries to disable with {} + override = {"post_admin_setup_05_create_manila_resources": {}} + + result = deep_merge(base, override) + + # Hook should be disabled (empty dict) + expected = {"post_admin_setup_05_create_manila_resources": {}} + self.assertEqual(result, expected) + + def test_multiple_hooks_disable(self): + """Test disabling multiple hooks simultaneously.""" + base = { + "post_admin_setup_05_create_manila_resources": { + "type": "playbook", + "source": "manila.yml", + }, + "post_deploy_90_Watcher_Download_needed_tools": { + "inventory": "/path/to/inventory", + "source": "/path/to/download_tools.yaml", + }, + "post_deploy_91_Watcher_Patch_Prometheus": { + "source": "/path/to/prometheus.yaml", + "type": "playbook", + }, + } + override = { + "post_admin_setup_05_create_manila_resources": {}, + "post_deploy_90_Watcher_Download_needed_tools": {}, + "post_deploy_91_Watcher_Patch_Prometheus": {}, + } + + result = deep_merge(base, override) + + expected = { + "post_admin_setup_05_create_manila_resources": {}, + "post_deploy_90_Watcher_Download_needed_tools": {}, + "post_deploy_91_Watcher_Patch_Prometheus": {}, + } + self.assertEqual(result, expected) + + def test_partial_hook_override_with_empty_dict(self): + """Test scenario where some hooks are disabled, others modified.""" + base = { + "hook_a": {"type": "playbook", "source": "a.yml"}, + "hook_b": {"type": "playbook", "source": "b.yml"}, + "hook_c": {"type": "playbook", "source": "c.yml"}, + } + override = { + "hook_a": {}, # Disable this one + "hook_b": {"type": "shell", "source": "b_modified.yml"}, # Modify + # hook_c not mentioned - should keep base + } + + result = deep_merge(base, override) + + expected = { + "hook_a": {}, + "hook_b": {"type": "shell", "source": "b_modified.yml"}, + "hook_c": {"type": "playbook", "source": "c.yml"}, + } + self.assertEqual(result, expected) + + def test_deeply_nested_empty_dict(self): + """Test empty dict override at multiple nesting levels.""" + base = {"level1": {"level2": {"level3": {"level4": {"key": "value"}}}}} + override = {"level1": {"level2": {"level3": {}}}} # Clear everything at level3 + + result = deep_merge(base, override) + + expected = {"level1": {"level2": {"level3": {}}}} + self.assertEqual(result, expected) + + def test_empty_dict_alongside_normal_merge(self): + """Test empty dict override mixed with normal dict merging.""" + base = { + "section_a": {"x": 1, "y": 2}, + "section_b": {"p": "old", "q": "old"}, + } + override = { + "section_a": {}, # Clear section_a + "section_b": {"q": "new", "r": "new"}, # Merge into section_b + } + + result = deep_merge(base, override) + + expected = { + "section_a": {}, + "section_b": {"p": "old", "q": "new", "r": "new"}, + } + self.assertEqual(result, expected) + + +if __name__ == "__main__": + unittest.main()