[BUG] Very minor type-checking issue in the shutdown() function of the salt.modules.win_system module #57804
Labels
Bug
broken, incorrect, or confusing behavior
P3
Priority 3
severity-low
4th level, cosemtic problems, work around exists
Milestone
Description
This is a very minor typing issue in
salt.modules.win_system
, and should likely be noted for the next person who ends up committing to this file.So, this is actually a universal issue and it's mostly because Python's lack of a typesystem forces each implementor to explicitly validate or "cast" the types that they're given as parameters. This is usually fine for internally called functions, but as saltstack exposes these functions directly to the user.. they should be type-checked so that users don't need to know Python in order to resolve these issues.
There's an enforcement of the parameter's type at the top of the function, but unfortunately it's only for Python2 which will soon go away. Whenever it does, that'll likely be a good time to fix issues similar to this.
However if these types of bugs are incredibly common, a universal issue could likely be discussed (probably as an SEP) after the ports-to-master project is completed and the code-base stabilizes. These issues can likely be remedied with a combination of two decorators. One decorator which validates types passed as parameters (and maybe the result), and another decorator which enforces the types of the parameters that are passed in from the user. If performance is a concern, some of them could be even disabled during production or affected by the loglevel.
I've done something close to the enforcement decorator in another project (https://github.com/arizvisa/ida-minsc/blob/master/base/_utils.py#L968) as the project is similar to salt in that internally defined functions are exposed to the user as part of the interface. This needed implicit string conversions to be done on the user's input before the function used them, and since the function was being directly exposed there wasn't any other way to wrap it before the user would dispatch into it.
An example for a decorator for
shutdown()
could look like@salt.utils.decorators.verify(message=str, timeout=int)
, or@salt.utils.decorators.result_dict(ret_code=int, name=str)
. Then for enforcing a type maybe@salt.utils.decorators.format(message="{!s}".format)
. Another benefit of doing something like this is that you can parse out the decorator in a test pretty easily with theast
module (or any linter api) in order to extract parameter type information and test all of the functions that are exposed as the "user-interface" get some automated coverage for these types of issues.This'd squash a large number of my issues such as #55708, #55944, and likely others.
Setup
Just need a windows minion, and it to be running salt w/ Python3.
Steps to Reproduce the behavior
Pretend that you know what you're doing, but instead pass an integral to
system.shutdown
as the first parameter thinking it's the timeout. Something like the following will do.Expected behavior
Likely a better error message possibly warning about which parameter is being passed as the incorrect type since it's only being enforced in Python2.
Versions Report
The text was updated successfully, but these errors were encountered: