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

[hurzhurz]

What does this PR do?

Fixes #65842 Module.run state returns invalid result when method returns a dict

module.run should also check for a key result in the return of the executed module to determin if it was successful.
But this only works for the legacy- and not the new-style.

This is because in the new-style, the _get_result function is always executed with a actually empty dict as the changes parameter, where it searches for the result key.

Also this changes parameter is not necessary as even, in the case of the legacy-style, it is effectively just a copy of the func_ret parameter.

[bdrx312]

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.

[hurzhurz]

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.

[bdrx312]

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.

[hurzhurz]

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.

[bdrx312]

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

[hurzhurz]

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.