SEP 24: Add sub_state_return #32
Conversation
ghost
requested review from
|
Should this also cover "internal" sub-states? There are a few states that call This talks a lot about the motivation and impact on |
Yeah, we wrote an abomination of the state that calls for 4 different states and we have to play around this very hard to be able to return correct results. |
|
@OrangeDog The original motivation for this was indeed because of idem and idem-cloud. |
| "sub_state_run": [ | ||
| { | ||
| "changes": {}, # A dictionary describing the changes made in the external state run | ||
| "result": None, # The external state run name | ||
| "comment": None, # Comment on the external state run | ||
| "duration": None, # Optional, the duration in seconds of the external state run | ||
| "start_time": None, # Optional, the timestamp of the external state run's start time | ||
| "low": { | ||
| "name": None, # The name of the state from the external state run | ||
| "state": None, # Name of the external state run | ||
| "__id__": None, # ID of the external state run | ||
| "fun": None, # The Function name from the external state run | ||
| }, | ||
| } | ||
| ], | ||
| } |
There was a problem hiding this comment.
Is this a good way for the return data to be formatted? I wanted to pass it through the state low data processing function as regular states. Any thoughts on the implementation here?
There was a problem hiding this comment.
This talks a lot about the motivation and impact on
idem, but what about the regular salt engine?
What are the internal and observable changes to e.g. the ansible states, and would they require e.g. changes to peoples' returners?
By default, they won't see any change. It won't break, but nobody will see anything fancy - it will look exactly how it does today. But if the ansible states/modules are updated to actually return the sub state return data? 🎉 then they would show up.
Basically this proposal is driven by idem, but can benefit anything that takes advantage of returning more structured data.
@Akm0d maybe it would also help to add a sample of the current output, as well as what the output would look like with this change applied - obviously using the default outputter, but perhaps JSON style as well.
Shouldn't part of the implementation plan be to do that then (and everywhere else applicable)? A PoC and an analysis of e.g. whether it's a breaking change. |
|
The PoC has been done with idem. It works and does not appear to be a breaking change. The door would be wide open for sub state returns to be applied everywhere else they would be useful. If the SEP is approved I could certainly open feature requests for sub state returns to be used in the ansiblegate state and all the other places it would be useful 👍 |
Yes, but I'm asking about the main salt engine, not idem. |
It's probably worthwhile doing a PoC with Ansible/etc. to confirm whether or not they can take advantage of these changes, and whether or not doing that would be a breaking change, though by adding return data I'm not sure how it would be -- unless one is depending on the absence of keys in the return, which seems like a bad idea 😬 But I don't agree that this SEP/feature should require that anything uses it, it should just make the functionality available -- and as you mentioned it shouldn't require breaking changes (unless absolutely necessary). |
|
@Akm0d btw apparently I didn't let you know - this is SEP 24, feel free to update your file! |
waynew
changed the title
sagetherage
added
Draft
|
Congratulations on your first PR being merged! 🎉 |
Add the ability for states to have
sub_state_runs so that state output from modules that can execute multiple states/SLSs/configurations/plays simultaneously can very clear and verbose.