Repository: saltstack/salt
Pull request: Add onchange configuration for script engine
### 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: https://github.com/saltstack/salt/issues/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.**
<!-- Please review the [test documentation](https://docs.saltproject.io/en/master/topics/tutorials/writing_tests.html) for details on how to implement tests into Salt's test suite. -->
- [x] Docs
- [x] Changelog - https://docs.saltproject.io/en/master/topics/development/changelog.html
- [x] Tests written/updated

### Commits signed with GPG?
Yes

Problem statement:
[FEATURE REQUEST] salt.engines.script: only fire event on data change
**Is your feature request related to a problem? Please describe.**
I wrote a script that derives data from an interface IP address and use the script engine to monitor its output. Only when the event data changes (on IP address change), I would like to initiate a reaction.

**Describe the solution you'd like**
Introduce an `onchange` configuration value that enables tracking the events returned by the script and only fires an event when the tag-specific data changes compared to the last run.

**Describe alternatives you've considered**
* track the state inside the script (cumbersome)
* write a custom engine (overkill)

There might be many more ways of dealing with this. Please point out if I'm missing something obvious since I'm not too familiar with the event system currently. `onchange` seems like a natural fit to me though.

Hints from issue discussion:

Commit message: Add more tests for script engine

Patch to review:
diff --git a/changelog/62867.added b/changelog/62867.added
new file mode 100644
index 000000000000..fd9ad1d278a0
--- /dev/null
+++ b/changelog/62867.added
@@ -0,0 +1 @@
+Added onchange configuration for script engine
diff --git a/salt/engines/script.py b/salt/engines/script.py
index 15628753d31b..670753011e11 100644
--- a/salt/engines/script.py
+++ b/salt/engines/script.py
@@ -10,13 +10,23 @@
           cmd: /some/script.py -a 1 -b 2
           output: json
           interval: 5
+          onchange: false
 
 Script engine configs:
 
-    cmd: Script or command to execute
-    output: Any available saltstack deserializer
-    interval: How often in seconds to execute the command
+cmd
+    Script or command to execute
 
+output
+    Any available saltstack deserializer
+
+interval
+    How often in seconds to execute the command
+
+onchange
+    .. versionadded:: 3006
+
+    Only fire an event if the tag-specific output changes. Defaults to False.
 """
 
 import logging
@@ -53,7 +63,7 @@ def _get_serializer(output):
         )
 
 
-def start(cmd, output="json", interval=1):
+def start(cmd, output="json", interval=1, onchange=False):
     """
     Parse stdout of a command and generate an event
 
@@ -80,6 +90,7 @@ def start(cmd, output="json", interval=1):
     :param cmd: The command to execute
     :param output: How to deserialize stdout of the script
     :param interval: How often to execute the script
+    :param onchange: Only fire an event if the tag-specific output changes
     """
     try:
         cmd = shlex.split(cmd)
@@ -96,7 +107,10 @@ def start(cmd, output="json", interval=1):
     else:
         fire_master = __salt__["event.send"]
 
-    while True:
+    if onchange:
+        events = {}
+
+    while _running():
 
         try:
             proc = subprocess.Popen(
@@ -116,8 +130,12 @@ def start(cmd, output="json", interval=1):
                     data["id"] = __opts__["id"]
 
                 if tag:
+                    if onchange and tag in events and events[tag] == data:
+                        continue
                     log.info("script engine firing event with tag %s", tag)
                     fire_master(tag=tag, data=data)
+                    if onchange:
+                        events[tag] = data
 
             log.debug("Closing script with pid %d", proc.pid)
             proc.stdout.close()
@@ -132,3 +150,8 @@ def start(cmd, output="json", interval=1):
                 proc.terminate()
 
         time.sleep(interval)
+
+
+# helper to test the start function
+def _running():
+    return True
diff --git a/tests/pytests/unit/engines/test_script.py b/tests/pytests/unit/engines/test_script.py
index 286fc0ff6012..65974c7a576a 100644
--- a/tests/pytests/unit/engines/test_script.py
+++ b/tests/pytests/unit/engines/test_script.py
@@ -2,17 +2,20 @@
 unit tests for the script engine
 """
 
+import copy
+
 import pytest
 
 import salt.config
 import salt.engines.script as script
 from salt.exceptions import CommandExecutionError
-from tests.support.mock import patch
+from tests.support.mock import Mock, patch
 
 
 @pytest.fixture
 def configure_loader_modules():
-    opts = salt.config.DEFAULT_MASTER_OPTS
+    opts = copy.deepcopy(salt.config.DEFAULT_MASTER_OPTS)
+    opts["id"] = "test"
     return {script: {"__opts__": opts}}
 
 
@@ -45,3 +48,114 @@ def test__read_stdout_terminates_properly():
         popen_mock.stdout.readline.return_value = b""
         with pytest.raises(StopIteration):
             next(script._read_stdout(popen_mock))
+
+
+@pytest.fixture()
+def serializer():
+    with patch("salt.engines.script._get_serializer", autospec=True) as get_serializer:
+        serializer = Mock()
+        get_serializer.return_value = serializer
+        serializer.deserialize.side_effect = lambda x: x
+        yield serializer
+
+
+@pytest.fixture(params=[1])
+def runs(request):
+    runs = Mock()
+    runs.side_effect = request.param * [True] + [False]
+    with patch("salt.engines.script._running", runs):
+        yield
+
+
+@pytest.fixture()
+def event_send():
+    event = Mock()
+    with patch("salt.utils.event.get_master_event") as get_master:
+        get_master.fire_event = event
+        with patch.dict(script.__salt__, {"event.send": event}):
+            yield event
+
+
+@pytest.fixture()
+def raw_event():
+    with patch("salt.engines.script._read_stdout") as stdout:
+        yield stdout
+
+
+@pytest.fixture()
+def proc():
+    with patch("salt.engines.script.subprocess.Popen") as popen:
+        proc = Mock()
+        proc.wait.return_value = False
+        proc.pid = 1337
+        popen.return_value = proc
+        yield
+
+
+@pytest.fixture()
+def sleep():
+    with patch("time.sleep"):
+        yield
+
+
+@pytest.fixture()
+def event():
+    return {"tag": "test", "data": {"foo": "bar", "id": "test"}}
+
+
+@pytest.fixture()
+def new_tag():
+    return {"tag": "testnew", "data": {"foo": "bar", "id": "test"}}
+
+
+@pytest.fixture()
+def new_event():
+    return {"tag": "test", "data": {"foo": "baz", "id": "test"}}
+
+
+@pytest.mark.usefixtures(
+    "proc", "serializer", "runs", "sleep", "event_send", "raw_event"
+)
+class TestStart:
+    def test_start(self, event, raw_event, event_send):
+        raw_event.return_value = [event]
+        script.start("cmd")
+        event_send.assert_called_once_with(tag=event["tag"], data=event["data"])
+
+    def test_start_multiple(self, event, new_event, raw_event, event_send):
+        raw_event.return_value = [event, new_event]
+        script.start("cmd")
+        assert event_send.call_count == 2
+        event_send.assert_any_call(tag=event["tag"], data=event["data"])
+        event_send.assert_any_call(tag=new_event["tag"], data=new_event["data"])
+
+    @pytest.mark.parametrize("runs", [10], indirect=True)
+    def test_start_onchange_no_change(self, event, raw_event, event_send):
+        raw_event.side_effect = 10 * [[event]]
+        script.start("cmd", onchange=True)
+        event_send.assert_called_once_with(tag=event["tag"], data=event["data"])
+
+    @pytest.mark.parametrize("runs", [10], indirect=True)
+    def test_start_onchange_no_change_multiple(
+        self, event, new_tag, raw_event, event_send
+    ):
+        raw_event.side_effect = 10 * [[event, new_tag]]
+        script.start("cmd", onchange=True)
+        assert event_send.call_count == 2
+        event_send.assert_any_call(tag=event["tag"], data=event["data"])
+        event_send.assert_any_call(tag=new_tag["tag"], data=new_tag["data"])
+
+    @pytest.mark.parametrize("runs", [8], indirect=True)
+    def test_start_onchange_with_change(self, event, new_event, raw_event, event_send):
+        raw_event.side_effect = 3 * [[event]] + 5 * [[new_event]]
+        script.start("cmd", onchange=True)
+        assert event_send.call_count == 2
+        event_send.assert_any_call(tag=event["tag"], data=event["data"])
+        event_send.assert_called_with(tag=new_event["tag"], data=new_event["data"])
+
+    @pytest.mark.parametrize("runs", [10], indirect=True)
+    def test_start_onchange_new_tag(self, event, new_tag, raw_event, event_send):
+        raw_event.side_effect = 5 * [[event]] + 5 * [[new_tag]]
+        script.start("cmd", onchange=True)
+        event_send.assert_any_call(tag=event["tag"], data=event["data"])
+        event_send.assert_called_with(tag=new_tag["tag"], data=new_tag["data"])


Code review comments:
diff_hunk: @@ -132,3 +150,8 @@ def start(cmd, output="json", interval=1):
                 proc.terminate()
 
         time.sleep(interval)
+
+
+# helper to test the start function
+def _running():
path: salt/engines/script.py
text: @user2:
@user1 any ideas on how to test this without adding this function?

@author:
I agree, don't like this as well. I tried introducing a global "constant", but since it is never called, patching it seems infeasible. I wrote the tests to demonstrate this works, we can always scrap them.

The only related post I could find was [this](https://deniscapeto.com/2021/03/06/how-to-test-a-while-true-in-python/), but that seems to need a class, I could not make it work on the module itself.

@user1:
I have some ideas but I'm AFK at the moment. I'm pretty sure I can put together some super reasonable tests without needing this, though

@user1:
I thought I remembered seeing something in another PR - https://github.com/saltstack/salt/blob/5dc28c7b0c45d61aed550bd3903f108bebbf90a5/tests/pytests/unit/cloud/clouds/test_hetzner.py#L447-L460 this timer should allow us to do a helpful thing here

@author:
In the meantime I refactored the script engine to make use of the solution I linked above, which feels a little less weird. Will push the commit now. I'm happy to learn though, so please feel free to point me in the right direction or take over the testing. :)