-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Add capability to specify state args as a dict #68368
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.
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
base: master
Are you sure you want to change the base?
Conversation
b2a27d9 to
a8b7362
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think we should update the docs to show the new dict method in addition to the list method.
Alright I'll look at updating docs when I get some time. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for creating this! Just some minor comments.
salt/state.py
Outdated
| K = TypeVar("K") | ||
| V = TypeVar("V") | ||
| Pair = tuple[K, V] | ||
| HighDataStateArgsDef = Union[Mapping[K, V], Iterable[Union[Mapping[K, V], str]]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| HighDataStateArgsDef = Union[Mapping[K, V], Iterable[Union[Mapping[K, V], str]]] | |
| HighDataStateArgsDef = Mapping[K, V] | Iterable[Mapping[K, V] | str] |
Minor suggestion: AFAIK 3008.0 is Python 3.10+ only, so you can use the | type operator. Unsure why pyupgrade does not catch this.
salt/state.py
Outdated
| self.states[ | ||
| "{}.{}".format(low["state"], low["fun"]) | ||
| ] # pylint: disable=W0106 | ||
| self.states[f"{low['state']}.{low['fun']}"] # pylint: disable=W0106 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: (inherited from the old code) This should have been # pylint: disable=expression-not-assigned (for clarity), but might need # pylint: disable=pointless-statement now. Maybe you can try removing the disable and see if it's necessary at all.
Also add some more type hints and change some format calls to f strings. Fix state retry tests with invalid assumptions of duration and comment.
7296a1f to
42fd957
Compare
Also add some more type hints and change some format calls to f strings. Fix state retry tests with invalid assumptions of duration and comment.
What does this PR do?
Adds the ability to specify arguments to a state as a dictionary instead of having to specify a list of individual key: value pairs
What issues does this PR fix or reference?
Fixes #68367
Previous Behavior
Could only specify arguments to a state like this
New Behavior
Can specify arguments to a state like this
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
No