Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3006.x] module.run: fix result detection from returned dict #67115

Open
wants to merge 1 commit into
base: 3006.x
Choose a base branch
from
Open

[3006.x] module.run: fix result detection from returned dict #67115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/65842.fixed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixed result detection of module.run from returned dict
38 changes: 18 additions & 20 deletions salt/states/module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ def _run(**kwargs):
func_ret = _call_function(
_func, returner=kwargs.get("returner"), func_args=kwargs.get(func)
)
if not _get_result(func_ret, ret["changes"].get("ret", {})):
if not _get_result(func_ret):
if isinstance(func_ret, dict):
failures.append(
"'{}' failed: {}".format(
Expand Down Expand Up @@ -658,31 +658,29 @@ def _legacy_run(name, **kwargs):
if kwargs["returner"] in returners:
returners[kwargs["returner"]](ret_ret)
ret["comment"] = f"Module function {name} executed"
ret["result"] = _get_result(mret, ret["changes"])
ret["result"] = _get_result(mret)

return ret


def _get_result(func_ret, changes):
def _get_result(func_ret):
res = True
# if mret is a dict and there is retcode and its non-zero
if isinstance(func_ret, dict) and func_ret.get("retcode", 0) != 0:
res = False
# if its a boolean, return that as the result
elif isinstance(func_ret, bool):
# if mret a boolean, return that as the result
if isinstance(func_ret, bool):
res = func_ret
else:
changes_ret = changes.get("ret", {})
if isinstance(changes_ret, dict):
if isinstance(changes_ret.get("result", {}), bool):
res = changes_ret.get("result", {})
elif changes_ret.get("retcode", 0) != 0:
res = False
# Explore dict in depth to determine if there is a
# 'result' key set to False which sets the global
# state result.
else:
res = _get_dict_result(changes_ret)
# if mret is a dict, check if certain keys exist
elif isinstance(func_ret, dict):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no longer an else case for when func_ret is not a bool nor a dict, the return will be True. The func_ret not being a bool nor dict is unexpected so should throw an exception or at least return False. Also we are checking isinstance(func_ret, dict) multiple times (also on line 669). We should just check once and have the func_ret.get("retcode", 0) != 0 check be inside it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was not really a plain else case before, as the else had the if isinstance(changes_ret, dict) inside which itself had no else case.
And I think it is ok or even wanted to have it return True in such cases: Not every execution module function does give a clear successful/unsuccessful indication.

Copy link
Contributor

@bdrx312 bdrx312 Jan 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not every execution module function does give a clear successful/unsuccessful indication.

In that case I would expect func_ret to be None which I recommend we handle in the way you deem correct, but an unexpected data type should either return the truth/bool value of it self (i.e. bool(func_ret) so empty list [] is False and list with value is True) or should throw an exception because we are getting an unexpected value. We should not do the worst case though of returning True that function was successful it wasn't and returned a bad value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there seems to be no hard standard for the return of execution modules. Unlike state modules.
Returning None or [] could simply mean that it had nothing to do.
For example saltutil.sync_modules does return a list of synced modules. But if there was nothing to sync, the list is empty, which is totally fine and actually a success.

However, there are some typical ways how some module functions indicate the succession: By a returning a bool or by dict keys result or retcode. Everything else would be a guess.
Using module.run is just some kind of a workaround anyway. If a proper determination of the result is needed, one should look for a state module or make one.

Also this PR is just meant to fix the limited (and often sufficient) way the function already had to determine the succession. Not to redesign it.

# if return key exists and is boolean, return that as the result
if isinstance(func_ret.get("result", {}), bool):
res = func_ret.get("result", {})
Comment on lines +674 to +675
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we determined the result is already in the dict, I recommend using [] instead of get. Also we don't use the default {} value on line 674 so I recommend using None instead:

if isinstance(func_ret.get("result"), bool):
    res = func_ret["result"]

alternatively we can use the newer python := assignment operator:

if isinstance(bool_res := func_ret.get("result"), bool):
    res = bool_res

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we determined the result is already in the dict, I recommend using [] instead of get. Also we don't use the default {} value on line 674 so I recommend using None instead:

if isinstance(func_ret.get("result"), bool):
    res = func_ret["result"]

I actually thought about that, but I didn't want to start refactoring the whole module.
And I just moved those lines and did not create or edit them.

alternatively we can use the newer python := assignment operator:

if isinstance(bool_res := func_ret.get("result"), bool):
    res = bool_res

While I think the assignment operator is pretty neat, I think it is better to not use it here.
Salt is sometimes still used with somewhat old python versions that don't support it yet. For example if you use salt-ssh, you need to rely on the python installation that already exists on the system.

# if retcode exists and it is non-zero, return False
elif func_ret.get("retcode", 0) != 0:
hurzhurz marked this conversation as resolved.
Show resolved Hide resolved
res = False
# Explore dict in depth to determine if there is a
# 'result' key set to False which sets the global
# state result.
else:
res = _get_dict_result(func_ret)

return res

Expand Down
17 changes: 17 additions & 0 deletions tests/unit/states/test_module.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,6 +421,23 @@ def test_module_run_missing_arg(self):
self.assertIn("world", ret["comment"])
self.assertIn("hello", ret["comment"])

def test_module_run_ret_result(self):
with patch.dict(module.__salt__, {CMD: _mocked_none_return}), patch.dict(
module.__opts__, {"use_superseded": ["module.run"]}
):
for val, res in [
(True, True),
(False, False),
({"result": True}, True),
({"result": False}, False),
({"retcode": 0}, True),
({"retcode": 1}, False),
({"bla": {"result": True}}, True),
({"bla": {"result": False}}, False),
]:
ret = module.run(**{CMD: [{"ret": val}]})
self.assertEqual(ret["result"], res)

def test_call_function_named_args(self):
"""
Test _call_function routine when params are named. Their position ordering should not matter.
Expand Down
Loading