Skip to content

Commit 8a0a23b

Browse files
Allow setting None for leaf nodes (#121)
* Add support for settings with None values * add tests + bugfix * revert changes on module_index * some Linting * Apply suggestions from code review Co-authored-by: Fabio Bertagna <33524186+DonGiovanni83@users.noreply.github.com> * revision (separate tests) * make the linter not cry --------- Co-authored-by: Fabio Bertagna <33524186+DonGiovanni83@users.noreply.github.com>
1 parent 4365c7e commit 8a0a23b

File tree

2 files changed

+53
-2
lines changed

2 files changed

+53
-2
lines changed

plugins/module_utils/config_utils.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -435,8 +435,12 @@ def set(self, value: str, setting: str) -> None:
435435
_setting: Element = self._config_xml_tree.find(xpath)
436436

437437
# If the element is present we will verify it's .text value
438-
elif _setting.text in [None, "", " "]:
439-
raise NotImplementedError("Currently only text settings supported")
438+
elif _setting.text is None or _setting.text.strip() == "":
439+
# check if setting has children
440+
if list(_setting):
441+
raise AttributeError(
442+
f"Cannot assign value to node '{_setting}' with child elements."
443+
)
440444

441445
_setting.text = value
442446

tests/unit/plugins/module_utils/test_config_utils.py

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,17 @@
5858
},
5959
},
6060
},
61+
"test_module_4": {
62+
"hasync_parent": "hasync",
63+
"remote_system_username": "hasync/username",
64+
"php_requirements": ["req_1", "req_2"],
65+
"configure_functions": {
66+
"test_configure_function": {
67+
"name": "test_configure_function",
68+
"configure_params": ["param_1"],
69+
},
70+
},
71+
},
6172
"missing_php_requirements": {
6273
"setting_1": "settings/one",
6374
"setting_2": "settings/two",
@@ -105,6 +116,9 @@
105116
<one>1</one>
106117
<two>2</two>
107118
</settings>
119+
<hasync>
120+
<username />
121+
</hasync>
108122
</opnsense>
109123
"""
110124

@@ -527,3 +541,36 @@ def test_set_with_missing_element(sample_config_path):
527541
},
528542
}
529543
new_config.save()
544+
545+
546+
def test_fail_set_on_parent_node(sample_config_path):
547+
"""
548+
Test case to verify that setting a value for a parent node will fail.
549+
550+
Args:
551+
- sample_config_path (str): The path to the temporary test configuration file.
552+
"""
553+
with OPNsenseModuleConfig(
554+
module_name="test_module_4",
555+
config_context_names=["test_module_4"],
556+
path=sample_config_path,
557+
check_mode=False,
558+
) as new_config:
559+
with pytest.raises(AttributeError):
560+
new_config.set("test", "hasync_parent")
561+
562+
563+
def test_success_set_on_empty_leaf_node(sample_config_path):
564+
"""
565+
Test case to verify that setting a leaf node with a value of None will succeed.
566+
- sample_config_path (str): The path to the temporary test configuration file.
567+
"""
568+
with OPNsenseModuleConfig(
569+
module_name="test_module_4",
570+
config_context_names=["test_module_4"],
571+
path=sample_config_path,
572+
check_mode=False,
573+
) as new_config:
574+
new_config.set("test", "remote_system_username")
575+
assert new_config.get("remote_system_username").text == "test"
576+
new_config.save()

0 commit comments

Comments
 (0)