Add onchange configuration for script engine - #62868
Conversation
d40d5a6 to
ede9794
Compare
ede9794 to
448b5f5
Compare
|
Looks good to me. @waynew can you also review please |
waynew
left a comment
waynew
left a comment
There was a problem hiding this comment.
Well, I thought that was just a comment but apparently GitHub decided posting on the comment page should open up a review 🤦
|
@lkubb so on today's Test Clinic I went ahead and added a fixture and a couple of tests. #62910 is my PR that adds the fixture, and you can find the tests I started on over here: https://github.com/saltstack/salt/compare/master...waynew:salt:engine-scripts-tests?expand=1 I think the timer approach is probably a reasonable way to go for this. |
|
@garethgreenaway I saw them, just waiting for his timeout fixture PR to be merged to revert my last commit and base off it. |
4354a49 to
637c404
Compare
* Sometimes, the timeout fails * Ensure that there are enough values in side_effect for small variations
|
I noticed more commits. Is this ready for review/merge now? or are you still waiting on #62910 ? |
|
I would consider this ready for review/merging now since I implemented a basic version of the timeout fixture @waynew proposed myself and reverted all the testing-related refactoring of the script engine. I'm not 100% certain the timeout fixture can be relied on though, in my testing it sometimes did not trigger. This PR contains a workaround for that issue specific to the script engine. |
|
thanks, just want @waynew to re-review here |
6 participants
What does this PR do?
Adds configuration for the script engine to only fire an event if the event tag-specific data changes compared to the last run (see the linked issue).
What issues does this PR fix or reference?
Fixes: #62867
Previous Behavior
Fires an event every time the script is run.
New Behavior
Can be configured to watch for changes between runs.
Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
Yes