From f39d84672bb38fed424d27f5efb9e39c3f841326 Mon Sep 17 00:00:00 2001 From: rebrowning Date: Thu, 21 Dec 2023 17:32:47 -0800 Subject: [PATCH 01/12] add a generic output schema to the local, remote, and winrm runners. St2 already appears to return the data in this format, this is simply formalizing the response so that validations can be added for commands leveraging these runners to better allow users of st2 to verify the integrity of the data their pipelines are processing --- .../local_runner/local_runner/runner.yaml | 40 +++++++++++++ .../remote_runner/remote_runner/runner.yaml | 40 +++++++++++++ .../winrm_runner/winrm_runner/runner.yaml | 60 +++++++++++++++++++ 3 files changed, 140 insertions(+) diff --git a/contrib/runners/local_runner/local_runner/runner.yaml b/contrib/runners/local_runner/local_runner/runner.yaml index 7463f46943..2a56edda99 100644 --- a/contrib/runners/local_runner/local_runner/runner.yaml +++ b/contrib/runners/local_runner/local_runner/runner.yaml @@ -33,6 +33,26 @@ description: Action timeout in seconds. Action will get killed if it doesn't finish in timeout seconds. type: integer + output_key: stdout + output_schema: + type: object + properties: + succeeded: + type: boolean + required: true + failed: + type: boolean + required: true + stdout: + anyOf: + - type: "object" + - type: "string" + required: true + stderr: + type: string + required: true + return_code: + type: integer - description: A runner to execute local actions as a fixed user. enabled: true name: local-shell-script @@ -74,3 +94,23 @@ description: Action timeout in seconds. Action will get killed if it doesn't finish in timeout seconds. type: integer + output_key: stdout + output_schema: + type: object + properties: + succeeded: + type: boolean + required: true + failed: + type: boolean + required: true + stdout: + anyOf: + - type: "object" + - type: "string" + required: true + stderr: + type: string + required: true + return_code: + type: integer \ No newline at end of file diff --git a/contrib/runners/remote_runner/remote_runner/runner.yaml b/contrib/runners/remote_runner/remote_runner/runner.yaml index 138973e218..3eaed94192 100644 --- a/contrib/runners/remote_runner/remote_runner/runner.yaml +++ b/contrib/runners/remote_runner/remote_runner/runner.yaml @@ -82,6 +82,26 @@ config is used. required: false type: string + output_key: stdout + output_schema: + type: object + properties: + succeeded: + type: boolean + required: true + failed: + type: boolean + required: true + stdout: + anyOf: + - type: "object" + - type: "string" + required: true + stderr: + type: string + required: true + return_code: + type: integer - description: A remote execution runner that executes actions as a fixed system user. enabled: true name: remote-shell-script @@ -162,3 +182,23 @@ config is used. required: false type: string + output_key: stdout + output_schema: + type: object + properties: + succeeded: + type: boolean + required: true + failed: + type: boolean + required: true + stdout: + anyOf: + - type: "object" + - type: "string" + required: true + stderr: + type: string + required: true + return_code: + type: integer \ No newline at end of file diff --git a/contrib/runners/winrm_runner/winrm_runner/runner.yaml b/contrib/runners/winrm_runner/winrm_runner/runner.yaml index 88a938c37f..7e1c47b475 100644 --- a/contrib/runners/winrm_runner/winrm_runner/runner.yaml +++ b/contrib/runners/winrm_runner/winrm_runner/runner.yaml @@ -66,6 +66,26 @@ CA bundle which comes from Mozilla. Verification using a custom CA bundle is not yet supported. Set to False to skip verification. type: boolean + output_key: stdout + output_schema: + type: object + properties: + succeeded: + type: boolean + required: true + failed: + type: boolean + required: true + stdout: + anyOf: + - type: "object" + - type: "string" + required: true + stderr: + type: string + required: true + return_code: + type: integer - description: A remote execution runner that executes PowerShell commands via WinRM on a remote host enabled: true name: winrm-ps-cmd @@ -134,6 +154,26 @@ CA bundle which comes from Mozilla. Verification using a custom CA bundle is not yet supported. Set to False to skip verification. type: boolean + output_key: stdout + output_schema: + type: object + properties: + succeeded: + type: boolean + required: true + failed: + type: boolean + required: true + stdout: + anyOf: + - type: "object" + - type: "string" + required: true + stderr: + type: string + required: true + return_code: + type: integer - description: A remote execution runner that executes PowerShell script via WinRM on a set of remote hosts enabled: true name: winrm-ps-script @@ -199,3 +239,23 @@ CA bundle which comes from Mozilla. Verification using a custom CA bundle is not yet supported. Set to False to skip verification. type: boolean + output_key: stdout + output_schema: + type: object + properties: + succeeded: + type: boolean + required: true + failed: + type: boolean + required: true + stdout: + anyOf: + - type: "object" + - type: "string" + required: true + stderr: + type: string + required: true + return_code: + type: integer \ No newline at end of file From c039c458c5c09f2c8f6288c1c5967f1ecb14710c Mon Sep 17 00:00:00 2001 From: rebrowning Date: Thu, 21 Dec 2023 22:19:27 -0800 Subject: [PATCH 02/12] updated several tests that were not returning a result that matches what st2 is currently outputting, so they were failing with the new schema being applied --- .../orquesta_runner/tests/unit/test_rerun.py | 2 +- .../tests/unit/test_with_items.py | 4 +- st2actions/tests/unit/test_policies.py | 37 +++++++++++++++++ .../unit/controllers/v1/test_executions.py | 40 +++++++++++-------- .../unit/services/test_workflow_rerun.py | 2 +- st2tests/st2tests/mocks/runners/runner.py | 18 ++++++++- 6 files changed, 80 insertions(+), 23 deletions(-) diff --git a/contrib/runners/orquesta_runner/tests/unit/test_rerun.py b/contrib/runners/orquesta_runner/tests/unit/test_rerun.py index 420b909e27..261f54812a 100644 --- a/contrib/runners/orquesta_runner/tests/unit/test_rerun.py +++ b/contrib/runners/orquesta_runner/tests/unit/test_rerun.py @@ -57,7 +57,7 @@ ) RUNNER_RESULT_SUCCEEDED = ( action_constants.LIVEACTION_STATUS_SUCCEEDED, - {"stdout": "foobar"}, + {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}, {}, ) diff --git a/contrib/runners/orquesta_runner/tests/unit/test_with_items.py b/contrib/runners/orquesta_runner/tests/unit/test_with_items.py index 8e8b67bd94..7a1d1ec549 100644 --- a/contrib/runners/orquesta_runner/tests/unit/test_with_items.py +++ b/contrib/runners/orquesta_runner/tests/unit/test_with_items.py @@ -56,13 +56,13 @@ RUNNER_RESULT_RUNNING = ( action_constants.LIVEACTION_STATUS_RUNNING, - {"stdout": "..."}, + {"stdout": "...", "stderr": ""}, {}, ) RUNNER_RESULT_SUCCEEDED = ( action_constants.LIVEACTION_STATUS_SUCCEEDED, - {"stdout": "..."}, + {"stdout": "...", "stderr": "", "succeeded": True, "failed": False}, {}, ) diff --git a/st2actions/tests/unit/test_policies.py b/st2actions/tests/unit/test_policies.py index 09a401e6b0..dedc285207 100644 --- a/st2actions/tests/unit/test_policies.py +++ b/st2actions/tests/unit/test_policies.py @@ -131,6 +131,43 @@ def test_apply(self): FakeConcurrencyApplicator.apply_after.assert_called_once_with(liveaction) RaiseExceptionApplicator.apply_after.assert_called_once_with(liveaction) + @mock.patch.object( + FakeConcurrencyApplicator, + "apply_before", + mock.MagicMock( + side_effect=FakeConcurrencyApplicator(None, None, threshold=3).apply_before + ), + ) + @mock.patch.object( + RaiseExceptionApplicator, + "apply_before", + mock.MagicMock(side_effect=RaiseExceptionApplicator(None, None).apply_before), + ) + @mock.patch.object( + FakeConcurrencyApplicator, + "apply_after", + mock.MagicMock( + side_effect=FakeConcurrencyApplicator(None, None, threshold=3).apply_after + ), + ) + @mock.patch.object( + RaiseExceptionApplicator, + "apply_after", + mock.MagicMock(side_effect=RaiseExceptionApplicator(None, None).apply_after), + ) + def test_apply_with_dict(self): + liveaction = LiveActionDB( + action="wolfpack.action-1", parameters={"actionstr": "dict_resp"} + ) + liveaction, _ = action_service.request(liveaction) + liveaction = self._wait_on_status( + liveaction, action_constants.LIVEACTION_STATUS_SUCCEEDED + ) + FakeConcurrencyApplicator.apply_before.assert_called_once_with(liveaction) + RaiseExceptionApplicator.apply_before.assert_called_once_with(liveaction) + FakeConcurrencyApplicator.apply_after.assert_called_once_with(liveaction) + RaiseExceptionApplicator.apply_after.assert_called_once_with(liveaction) + @mock.patch.object( FakeConcurrencyApplicator, "get_threshold", mock.MagicMock(return_value=0) ) diff --git a/st2api/tests/unit/controllers/v1/test_executions.py b/st2api/tests/unit/controllers/v1/test_executions.py index ea5e45b6fe..e54a8f4fb8 100644 --- a/st2api/tests/unit/controllers/v1/test_executions.py +++ b/st2api/tests/unit/controllers/v1/test_executions.py @@ -453,7 +453,7 @@ def test_get_one_max_result_size_query_parameter(self): # Update it with the result (this populates result and result size attributes) data = { - "result": {"fooo": "a" * 1000}, + "result": {"stdout": {"fooo": "a" * 1000}, "succeeded": True, "failed": False, "stderr": ""}, "status": "succeeded", } actual_result_size = len(json_encode(data["result"])) @@ -1289,16 +1289,16 @@ def test_put_status_and_result(self): self.assertEqual(post_resp.status_int, 201) execution_id = self._get_actionexecution_id(post_resp) - updates = {"status": "succeeded", "result": {"stdout": "foobar"}} + updates = {"status": "succeeded", "result": {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}} put_resp = self._do_put(execution_id, updates) self.assertEqual(put_resp.status_int, 200) self.assertEqual(put_resp.json["status"], "succeeded") - self.assertDictEqual(put_resp.json["result"], {"stdout": "foobar"}) + self.assertDictEqual(put_resp.json["result"], {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}) get_resp = self._do_get_one(execution_id) self.assertEqual(get_resp.status_int, 200) self.assertEqual(get_resp.json["status"], "succeeded") - self.assertDictEqual(get_resp.json["result"], {"stdout": "foobar"}) + self.assertDictEqual(get_resp.json["result"], {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}) def test_put_bad_state(self): post_resp = self._do_post(LIVE_ACTION_1) @@ -1337,11 +1337,11 @@ def test_put_status_to_completed_execution(self): self.assertEqual(post_resp.status_int, 201) execution_id = self._get_actionexecution_id(post_resp) - updates = {"status": "succeeded", "result": {"stdout": "foobar"}} + updates = {"status": "succeeded", "result": {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}} put_resp = self._do_put(execution_id, updates) self.assertEqual(put_resp.status_int, 200) self.assertEqual(put_resp.json["status"], "succeeded") - self.assertDictEqual(put_resp.json["result"], {"stdout": "foobar"}) + self.assertDictEqual(put_resp.json["result"], {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}) updates = {"status": "abandoned"} put_resp = self._do_put(execution_id, updates, expect_errors=True) @@ -1353,7 +1353,7 @@ def test_put_execution_missing_liveaction(self): self.assertEqual(post_resp.status_int, 201) execution_id = self._get_actionexecution_id(post_resp) - updates = {"status": "succeeded", "result": {"stdout": "foobar"}} + updates = {"status": "succeeded", "result": {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}} put_resp = self._do_put(execution_id, updates, expect_errors=True) self.assertEqual(put_resp.status_int, 500) @@ -1620,22 +1620,22 @@ def test_get_raw_result(self): execution_id = self._get_actionexecution_id(get_resp) updates = { "status": "succeeded", - "result": {"stdout": "foobar", "stderr": "barfoo"}, + "result": {"stdout": "foobar", "stderr": "barfoo", "succeeded": True, "failed": False}, } put_resp = self._do_put(execution_id, updates) self.assertEqual(put_resp.status_int, 200) self.assertEqual(put_resp.json["status"], "succeeded") self.assertDictEqual( - put_resp.json["result"], {"stdout": "foobar", "stderr": "barfoo"} + put_resp.json["result"], {"stdout": "foobar", "stderr": "barfoo", "succeeded": True, "failed": False} ) self.assertEqual( - put_resp.json["result_size"], len('{"stdout":"foobar","stderr":"barfoo"}') + put_resp.json["result_size"], len('{"stdout":"foobar","stderr":"barfoo","succeeded":true,"failed":false}') ) # 1. download=False, compress=False, pretty_format=False get_resp = self.app.get("/v1/executions/%s/result" % (execution_id)) self.assertEqual(get_resp.headers["Content-Type"], "text/json") - self.assertEqual(get_resp.body, b'{"stdout":"foobar","stderr":"barfoo"}') + self.assertEqual(get_resp.body, b'{"stdout":"foobar","stderr":"barfoo","succeeded":true,"failed":false}') # 2. download=False, compress=False, pretty_format=True get_resp = self.app.get( @@ -1644,7 +1644,9 @@ def test_get_raw_result(self): expected_result = b""" { "stdout": "foobar", - "stderr": "barfoo" + "stderr": "barfoo", + "succeeded": true, + "failed": false }""".strip() self.assertEqual(get_resp.headers["Content-Type"], "text/json") self.assertEqual(get_resp.body, expected_result) @@ -1653,7 +1655,7 @@ def test_get_raw_result(self): # NOTE: webtest auto decompresses the result get_resp = self.app.get("/v1/executions/%s/result?compress=1" % (execution_id)) self.assertEqual(get_resp.headers["Content-Type"], "application/x-gzip") - self.assertEqual(get_resp.body, b'{"stdout":"foobar","stderr":"barfoo"}') + self.assertEqual(get_resp.body, b'{"stdout":"foobar","stderr":"barfoo","succeeded":true,"failed":false}') # 4. download=True, compress=False, pretty_format=False get_resp = self.app.get("/v1/executions/%s/result?download=1" % (execution_id)) @@ -1662,7 +1664,7 @@ def test_get_raw_result(self): get_resp.headers["Content-Disposition"], "attachment; filename=execution_%s_result.json" % (execution_id), ) - self.assertEqual(get_resp.body, b'{"stdout":"foobar","stderr":"barfoo"}') + self.assertEqual(get_resp.body, b'{"stdout":"foobar","stderr":"barfoo","succeeded":true,"failed":false}') # 5. download=True, compress=False, pretty_format=True get_resp = self.app.get( @@ -1671,7 +1673,9 @@ def test_get_raw_result(self): expected_result = b""" { "stdout": "foobar", - "stderr": "barfoo" + "stderr": "barfoo", + "succeeded": true, + "failed": false }""".strip() self.assertEqual(get_resp.headers["Content-Type"], "text/json") @@ -1689,7 +1693,9 @@ def test_get_raw_result(self): expected_result = b""" { "stdout": "foobar", - "stderr": "barfoo" + "stderr": "barfoo", + "succeeded": true, + "failed": false }""".strip() self.assertEqual(get_resp.headers["Content-Type"], "application/x-gzip") @@ -1782,7 +1788,7 @@ def test_get_single_attribute_success(self): data = {} data["status"] = action_constants.LIVEACTION_STATUS_SUCCEEDED - data["result"] = {"foo": "bar"} + data["result"] = {"foo": "bar", "stdout": "hello world", "stderr": "", "succeeded": True, "failed": False} resp = self.app.put_json("/v1/executions/%s" % (exec_id), data) self.assertEqual(resp.status_int, 200) diff --git a/st2common/tests/unit/services/test_workflow_rerun.py b/st2common/tests/unit/services/test_workflow_rerun.py index c91e140d6c..0aa5cbaad5 100644 --- a/st2common/tests/unit/services/test_workflow_rerun.py +++ b/st2common/tests/unit/services/test_workflow_rerun.py @@ -51,7 +51,7 @@ RUNNER_RESULT_FAILED = (action_constants.LIVEACTION_STATUS_FAILED, {}, {}) RUNNER_RESULT_SUCCEEDED = ( action_constants.LIVEACTION_STATUS_SUCCEEDED, - {"stdout": "foobar"}, + {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}, {}, ) diff --git a/st2tests/st2tests/mocks/runners/runner.py b/st2tests/st2tests/mocks/runners/runner.py index b89b75b712..a64cf02820 100644 --- a/st2tests/st2tests/mocks/runners/runner.py +++ b/st2tests/st2tests/mocks/runners/runner.py @@ -29,7 +29,6 @@ def get_runner(config=None): class MockActionRunner(ActionRunner): def __init__(self): super(MockActionRunner, self).__init__(runner_id="1") - self.pre_run_called = False self.run_called = False self.post_run_called = False @@ -45,7 +44,22 @@ def run(self, action_params): if self.runner_parameters.get("raise", False): raise Exception("Raise required.") - default_result = {"ran": True, "action_params": action_params} + default_result = { + "ran": True, + "action_params": action_params, + "failed": False, + "stdout": "res", + "stderr": "", + "succeeded": True + } + if action_params.get("actionstr", "") == "dict_resp": + default_result["stdout"] = { + "key": "value", + "key2": { + "sk1": "v1" + } + } + default_context = {"third_party_system": {"ref_id": "1234"}} status = self.runner_parameters.get("mock_status", LIVEACTION_STATUS_SUCCEEDED) From 05ff62fd5c0f1d4293adad1229cabc4bb9de37ac Mon Sep 17 00:00:00 2001 From: rebrowning Date: Thu, 21 Dec 2023 22:37:57 -0800 Subject: [PATCH 03/12] fix some linting tests also add a little bit more verbose output so it's easier to find the issues... That flag can be removed if necessary --- Makefile | 4 +- .../unit/controllers/v1/test_executions.py | 95 ++++++++++++++++--- st2tests/st2tests/mocks/runners/runner.py | 9 +- 3 files changed, 85 insertions(+), 23 deletions(-) diff --git a/Makefile b/Makefile index 84679bceda..382ef9220d 100644 --- a/Makefile +++ b/Makefile @@ -377,7 +377,7 @@ black: requirements .black-check echo "==========================================================="; \ echo "Running black on" $$component; \ echo "==========================================================="; \ - . $(VIRTUALENV_DIR)/bin/activate ; black --check --config pyproject.toml $$component/ || exit 1; \ + . $(VIRTUALENV_DIR)/bin/activate ; black --diff --check --config pyproject.toml $$component/ || exit 1; \ if [ -d "$$component/bin" ]; then \ . $(VIRTUALENV_DIR)/bin/activate ; black $$(grep -rl '^#!/.*python' $$component/bin) || exit 1; \ fi \ @@ -387,7 +387,7 @@ black: requirements .black-check echo "==========================================================="; \ echo "Running black on" $$component; \ echo "==========================================================="; \ - . $(VIRTUALENV_DIR)/bin/activate ; black --check --config pyproject.toml $$component/ || exit 1; \ + . $(VIRTUALENV_DIR)/bin/activate ; black --diff --check --config pyproject.toml $$component/ || exit 1; \ if [ -d "$$component/bin" ]; then \ . $(VIRTUALENV_DIR)/bin/activate ; black $$(grep -rl '^#!/.*python' $$component/bin) || exit 1; \ fi \ diff --git a/st2api/tests/unit/controllers/v1/test_executions.py b/st2api/tests/unit/controllers/v1/test_executions.py index e54a8f4fb8..56595152fb 100644 --- a/st2api/tests/unit/controllers/v1/test_executions.py +++ b/st2api/tests/unit/controllers/v1/test_executions.py @@ -453,7 +453,12 @@ def test_get_one_max_result_size_query_parameter(self): # Update it with the result (this populates result and result size attributes) data = { - "result": {"stdout": {"fooo": "a" * 1000}, "succeeded": True, "failed": False, "stderr": ""}, + "result": { + "stdout": {"fooo": "a" * 1000}, + "succeeded": True, + "failed": False, + "stderr": "", + }, "status": "succeeded", } actual_result_size = len(json_encode(data["result"])) @@ -1289,16 +1294,30 @@ def test_put_status_and_result(self): self.assertEqual(post_resp.status_int, 201) execution_id = self._get_actionexecution_id(post_resp) - updates = {"status": "succeeded", "result": {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}} + updates = { + "status": "succeeded", + "result": { + "stdout": "foobar", + "succeeded": True, + "failed": False, + "stderr": "", + }, + } put_resp = self._do_put(execution_id, updates) self.assertEqual(put_resp.status_int, 200) self.assertEqual(put_resp.json["status"], "succeeded") - self.assertDictEqual(put_resp.json["result"], {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}) + self.assertDictEqual( + put_resp.json["result"], + {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}, + ) get_resp = self._do_get_one(execution_id) self.assertEqual(get_resp.status_int, 200) self.assertEqual(get_resp.json["status"], "succeeded") - self.assertDictEqual(get_resp.json["result"], {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}) + self.assertDictEqual( + get_resp.json["result"], + {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}, + ) def test_put_bad_state(self): post_resp = self._do_post(LIVE_ACTION_1) @@ -1337,11 +1356,22 @@ def test_put_status_to_completed_execution(self): self.assertEqual(post_resp.status_int, 201) execution_id = self._get_actionexecution_id(post_resp) - updates = {"status": "succeeded", "result": {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}} + updates = { + "status": "succeeded", + "result": { + "stdout": "foobar", + "succeeded": True, + "failed": False, + "stderr": "", + }, + } put_resp = self._do_put(execution_id, updates) self.assertEqual(put_resp.status_int, 200) self.assertEqual(put_resp.json["status"], "succeeded") - self.assertDictEqual(put_resp.json["result"], {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}) + self.assertDictEqual( + put_resp.json["result"], + {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}, + ) updates = {"status": "abandoned"} put_resp = self._do_put(execution_id, updates, expect_errors=True) @@ -1353,7 +1383,15 @@ def test_put_execution_missing_liveaction(self): self.assertEqual(post_resp.status_int, 201) execution_id = self._get_actionexecution_id(post_resp) - updates = {"status": "succeeded", "result": {"stdout": "foobar", "succeeded": True, "failed": False, "stderr": ""}} + updates = { + "status": "succeeded", + "result": { + "stdout": "foobar", + "succeeded": True, + "failed": False, + "stderr": "", + }, + } put_resp = self._do_put(execution_id, updates, expect_errors=True) self.assertEqual(put_resp.status_int, 500) @@ -1620,22 +1658,39 @@ def test_get_raw_result(self): execution_id = self._get_actionexecution_id(get_resp) updates = { "status": "succeeded", - "result": {"stdout": "foobar", "stderr": "barfoo", "succeeded": True, "failed": False}, + "result": { + "stdout": "foobar", + "stderr": "barfoo", + "succeeded": True, + "failed": False, + }, } put_resp = self._do_put(execution_id, updates) self.assertEqual(put_resp.status_int, 200) self.assertEqual(put_resp.json["status"], "succeeded") self.assertDictEqual( - put_resp.json["result"], {"stdout": "foobar", "stderr": "barfoo", "succeeded": True, "failed": False} + put_resp.json["result"], + { + "stdout": "foobar", + "stderr": "barfoo", + "succeeded": True, + "failed": False, + }, ) self.assertEqual( - put_resp.json["result_size"], len('{"stdout":"foobar","stderr":"barfoo","succeeded":true,"failed":false}') + put_resp.json["result_size"], + len( + '{"stdout":"foobar","stderr":"barfoo","succeeded":true,"failed":false}' + ), ) # 1. download=False, compress=False, pretty_format=False get_resp = self.app.get("/v1/executions/%s/result" % (execution_id)) self.assertEqual(get_resp.headers["Content-Type"], "text/json") - self.assertEqual(get_resp.body, b'{"stdout":"foobar","stderr":"barfoo","succeeded":true,"failed":false}') + self.assertEqual( + get_resp.body, + b'{"stdout":"foobar","stderr":"barfoo","succeeded":true,"failed":false}', + ) # 2. download=False, compress=False, pretty_format=True get_resp = self.app.get( @@ -1655,7 +1710,10 @@ def test_get_raw_result(self): # NOTE: webtest auto decompresses the result get_resp = self.app.get("/v1/executions/%s/result?compress=1" % (execution_id)) self.assertEqual(get_resp.headers["Content-Type"], "application/x-gzip") - self.assertEqual(get_resp.body, b'{"stdout":"foobar","stderr":"barfoo","succeeded":true,"failed":false}') + self.assertEqual( + get_resp.body, + b'{"stdout":"foobar","stderr":"barfoo","succeeded":true,"failed":false}', + ) # 4. download=True, compress=False, pretty_format=False get_resp = self.app.get("/v1/executions/%s/result?download=1" % (execution_id)) @@ -1664,7 +1722,10 @@ def test_get_raw_result(self): get_resp.headers["Content-Disposition"], "attachment; filename=execution_%s_result.json" % (execution_id), ) - self.assertEqual(get_resp.body, b'{"stdout":"foobar","stderr":"barfoo","succeeded":true,"failed":false}') + self.assertEqual( + get_resp.body, + b'{"stdout":"foobar","stderr":"barfoo","succeeded":true,"failed":false}', + ) # 5. download=True, compress=False, pretty_format=True get_resp = self.app.get( @@ -1788,7 +1849,13 @@ def test_get_single_attribute_success(self): data = {} data["status"] = action_constants.LIVEACTION_STATUS_SUCCEEDED - data["result"] = {"foo": "bar", "stdout": "hello world", "stderr": "", "succeeded": True, "failed": False} + data["result"] = { + "foo": "bar", + "stdout": "hello world", + "stderr": "", + "succeeded": True, + "failed": False, + } resp = self.app.put_json("/v1/executions/%s" % (exec_id), data) self.assertEqual(resp.status_int, 200) diff --git a/st2tests/st2tests/mocks/runners/runner.py b/st2tests/st2tests/mocks/runners/runner.py index a64cf02820..0a6603c537 100644 --- a/st2tests/st2tests/mocks/runners/runner.py +++ b/st2tests/st2tests/mocks/runners/runner.py @@ -50,15 +50,10 @@ def run(self, action_params): "failed": False, "stdout": "res", "stderr": "", - "succeeded": True + "succeeded": True, } if action_params.get("actionstr", "") == "dict_resp": - default_result["stdout"] = { - "key": "value", - "key2": { - "sk1": "v1" - } - } + default_result["stdout"] = {"key": "value", "key2": {"sk1": "v1"}} default_context = {"third_party_system": {"ref_id": "1234"}} From 874ab026d929c8bbed167475bef7022343c3b038 Mon Sep 17 00:00:00 2001 From: rebrowning Date: Fri, 12 Jan 2024 10:39:42 -0800 Subject: [PATCH 04/12] update the cchangelog --- CHANGELOG.rst | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index efb07a459e..9fa18fa030 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -126,6 +126,12 @@ Added * Cherry-pick changes to runners.sh from st2-packages git repo. #6302 Cherry-picked by @cognifloyd +Added +~~~~~ +* add a schema to shell and winrm runners to allow validations to be executed againt their payloads. This means that shell or winrm jobs + that return a json object will be parsed into a dict to allow for field validations to be performed. Non json string resonses are still + handled as normal #6102 + 3.8.1 - December 13, 2023 ------------------------- Fixed From fc2f6100d53a2a18d2c9e451a431c6db76ce07d2 Mon Sep 17 00:00:00 2001 From: rebrowning Date: Fri, 12 Jan 2024 11:15:24 -0800 Subject: [PATCH 05/12] see if these additional options work to cover additional types in tests --- contrib/runners/local_runner/local_runner/runner.yaml | 4 ++++ contrib/runners/remote_runner/remote_runner/runner.yaml | 4 ++++ contrib/runners/winrm_runner/winrm_runner/runner.yaml | 6 ++++++ 3 files changed, 14 insertions(+) diff --git a/contrib/runners/local_runner/local_runner/runner.yaml b/contrib/runners/local_runner/local_runner/runner.yaml index 2a56edda99..e4c173ca44 100644 --- a/contrib/runners/local_runner/local_runner/runner.yaml +++ b/contrib/runners/local_runner/local_runner/runner.yaml @@ -47,6 +47,8 @@ anyOf: - type: "object" - type: "string" + - type: "list" + - type: "number" required: true stderr: type: string @@ -108,6 +110,8 @@ anyOf: - type: "object" - type: "string" + - type: "list" + - type: "number" required: true stderr: type: string diff --git a/contrib/runners/remote_runner/remote_runner/runner.yaml b/contrib/runners/remote_runner/remote_runner/runner.yaml index 3eaed94192..5cbe628d87 100644 --- a/contrib/runners/remote_runner/remote_runner/runner.yaml +++ b/contrib/runners/remote_runner/remote_runner/runner.yaml @@ -96,6 +96,8 @@ anyOf: - type: "object" - type: "string" + - type: "list" + - type: "number" required: true stderr: type: string @@ -196,6 +198,8 @@ anyOf: - type: "object" - type: "string" + - type: "list" + - type: "number" required: true stderr: type: string diff --git a/contrib/runners/winrm_runner/winrm_runner/runner.yaml b/contrib/runners/winrm_runner/winrm_runner/runner.yaml index 7e1c47b475..d8f6d2cb73 100644 --- a/contrib/runners/winrm_runner/winrm_runner/runner.yaml +++ b/contrib/runners/winrm_runner/winrm_runner/runner.yaml @@ -80,6 +80,8 @@ anyOf: - type: "object" - type: "string" + - type: "list" + - type: "number" required: true stderr: type: string @@ -168,6 +170,8 @@ anyOf: - type: "object" - type: "string" + - type: "list" + - type: "number" required: true stderr: type: string @@ -253,6 +257,8 @@ anyOf: - type: "object" - type: "string" + - type: "list" + - type: "number" required: true stderr: type: string From ff8a6d821eab31dd86132177a474bca9af41afbf Mon Sep 17 00:00:00 2001 From: rebrowning Date: Fri, 12 Jan 2024 11:21:37 -0800 Subject: [PATCH 06/12] fix this type --- contrib/runners/local_runner/local_runner/runner.yaml | 4 ++-- contrib/runners/remote_runner/remote_runner/runner.yaml | 4 ++-- contrib/runners/winrm_runner/winrm_runner/runner.yaml | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/contrib/runners/local_runner/local_runner/runner.yaml b/contrib/runners/local_runner/local_runner/runner.yaml index e4c173ca44..2ec3f73c01 100644 --- a/contrib/runners/local_runner/local_runner/runner.yaml +++ b/contrib/runners/local_runner/local_runner/runner.yaml @@ -47,7 +47,7 @@ anyOf: - type: "object" - type: "string" - - type: "list" + - type: "array" - type: "number" required: true stderr: @@ -110,7 +110,7 @@ anyOf: - type: "object" - type: "string" - - type: "list" + - type: "array" - type: "number" required: true stderr: diff --git a/contrib/runners/remote_runner/remote_runner/runner.yaml b/contrib/runners/remote_runner/remote_runner/runner.yaml index 5cbe628d87..ab869385ab 100644 --- a/contrib/runners/remote_runner/remote_runner/runner.yaml +++ b/contrib/runners/remote_runner/remote_runner/runner.yaml @@ -96,7 +96,7 @@ anyOf: - type: "object" - type: "string" - - type: "list" + - type: "array" - type: "number" required: true stderr: @@ -198,7 +198,7 @@ anyOf: - type: "object" - type: "string" - - type: "list" + - type: "array" - type: "number" required: true stderr: diff --git a/contrib/runners/winrm_runner/winrm_runner/runner.yaml b/contrib/runners/winrm_runner/winrm_runner/runner.yaml index d8f6d2cb73..d560f973da 100644 --- a/contrib/runners/winrm_runner/winrm_runner/runner.yaml +++ b/contrib/runners/winrm_runner/winrm_runner/runner.yaml @@ -80,7 +80,7 @@ anyOf: - type: "object" - type: "string" - - type: "list" + - type: "array" - type: "number" required: true stderr: @@ -170,7 +170,7 @@ anyOf: - type: "object" - type: "string" - - type: "list" + - type: "array" - type: "number" required: true stderr: @@ -257,7 +257,7 @@ anyOf: - type: "object" - type: "string" - - type: "list" + - type: "array" - type: "number" required: true stderr: From 62f41cbddee36d4d51b94f57b0adf80ed14b0d9d Mon Sep 17 00:00:00 2001 From: rebrowning Date: Fri, 12 Jan 2024 11:36:44 -0800 Subject: [PATCH 07/12] adjust which pieces are required --- contrib/runners/local_runner/local_runner/runner.yaml | 6 ------ contrib/runners/remote_runner/remote_runner/runner.yaml | 6 ------ contrib/runners/winrm_runner/winrm_runner/runner.yaml | 9 --------- 3 files changed, 21 deletions(-) diff --git a/contrib/runners/local_runner/local_runner/runner.yaml b/contrib/runners/local_runner/local_runner/runner.yaml index 2ec3f73c01..25cd153edd 100644 --- a/contrib/runners/local_runner/local_runner/runner.yaml +++ b/contrib/runners/local_runner/local_runner/runner.yaml @@ -39,10 +39,8 @@ properties: succeeded: type: boolean - required: true failed: type: boolean - required: true stdout: anyOf: - type: "object" @@ -52,7 +50,6 @@ required: true stderr: type: string - required: true return_code: type: integer - description: A runner to execute local actions as a fixed user. @@ -102,10 +99,8 @@ properties: succeeded: type: boolean - required: true failed: type: boolean - required: true stdout: anyOf: - type: "object" @@ -115,6 +110,5 @@ required: true stderr: type: string - required: true return_code: type: integer \ No newline at end of file diff --git a/contrib/runners/remote_runner/remote_runner/runner.yaml b/contrib/runners/remote_runner/remote_runner/runner.yaml index ab869385ab..b4cdc0d5e1 100644 --- a/contrib/runners/remote_runner/remote_runner/runner.yaml +++ b/contrib/runners/remote_runner/remote_runner/runner.yaml @@ -88,10 +88,8 @@ properties: succeeded: type: boolean - required: true failed: type: boolean - required: true stdout: anyOf: - type: "object" @@ -101,7 +99,6 @@ required: true stderr: type: string - required: true return_code: type: integer - description: A remote execution runner that executes actions as a fixed system user. @@ -190,10 +187,8 @@ properties: succeeded: type: boolean - required: true failed: type: boolean - required: true stdout: anyOf: - type: "object" @@ -203,6 +198,5 @@ required: true stderr: type: string - required: true return_code: type: integer \ No newline at end of file diff --git a/contrib/runners/winrm_runner/winrm_runner/runner.yaml b/contrib/runners/winrm_runner/winrm_runner/runner.yaml index d560f973da..30dd722483 100644 --- a/contrib/runners/winrm_runner/winrm_runner/runner.yaml +++ b/contrib/runners/winrm_runner/winrm_runner/runner.yaml @@ -72,10 +72,8 @@ properties: succeeded: type: boolean - required: true failed: type: boolean - required: true stdout: anyOf: - type: "object" @@ -85,7 +83,6 @@ required: true stderr: type: string - required: true return_code: type: integer - description: A remote execution runner that executes PowerShell commands via WinRM on a remote host @@ -162,10 +159,8 @@ properties: succeeded: type: boolean - required: true failed: type: boolean - required: true stdout: anyOf: - type: "object" @@ -175,7 +170,6 @@ required: true stderr: type: string - required: true return_code: type: integer - description: A remote execution runner that executes PowerShell script via WinRM on a set of remote hosts @@ -249,10 +243,8 @@ properties: succeeded: type: boolean - required: true failed: type: boolean - required: true stdout: anyOf: - type: "object" @@ -262,6 +254,5 @@ required: true stderr: type: string - required: true return_code: type: integer \ No newline at end of file From dfedce77490c179ed0df71a1b1263ffe3e9d3497 Mon Sep 17 00:00:00 2001 From: rebrowning Date: Sat, 13 Jan 2024 12:58:53 -0800 Subject: [PATCH 08/12] remove the required field --- contrib/runners/local_runner/local_runner/runner.yaml | 2 -- contrib/runners/remote_runner/remote_runner/runner.yaml | 2 -- contrib/runners/winrm_runner/winrm_runner/runner.yaml | 3 --- 3 files changed, 7 deletions(-) diff --git a/contrib/runners/local_runner/local_runner/runner.yaml b/contrib/runners/local_runner/local_runner/runner.yaml index 25cd153edd..f2eec20966 100644 --- a/contrib/runners/local_runner/local_runner/runner.yaml +++ b/contrib/runners/local_runner/local_runner/runner.yaml @@ -47,7 +47,6 @@ - type: "string" - type: "array" - type: "number" - required: true stderr: type: string return_code: @@ -107,7 +106,6 @@ - type: "string" - type: "array" - type: "number" - required: true stderr: type: string return_code: diff --git a/contrib/runners/remote_runner/remote_runner/runner.yaml b/contrib/runners/remote_runner/remote_runner/runner.yaml index b4cdc0d5e1..807ec27d67 100644 --- a/contrib/runners/remote_runner/remote_runner/runner.yaml +++ b/contrib/runners/remote_runner/remote_runner/runner.yaml @@ -96,7 +96,6 @@ - type: "string" - type: "array" - type: "number" - required: true stderr: type: string return_code: @@ -195,7 +194,6 @@ - type: "string" - type: "array" - type: "number" - required: true stderr: type: string return_code: diff --git a/contrib/runners/winrm_runner/winrm_runner/runner.yaml b/contrib/runners/winrm_runner/winrm_runner/runner.yaml index 30dd722483..5727c9457b 100644 --- a/contrib/runners/winrm_runner/winrm_runner/runner.yaml +++ b/contrib/runners/winrm_runner/winrm_runner/runner.yaml @@ -80,7 +80,6 @@ - type: "string" - type: "array" - type: "number" - required: true stderr: type: string return_code: @@ -167,7 +166,6 @@ - type: "string" - type: "array" - type: "number" - required: true stderr: type: string return_code: @@ -251,7 +249,6 @@ - type: "string" - type: "array" - type: "number" - required: true stderr: type: string return_code: From 89d6cdc4146468efceebbb5afdbc18b3a7fd9486 Mon Sep 17 00:00:00 2001 From: Robert Browning Date: Tue, 14 Oct 2025 10:58:33 -0700 Subject: [PATCH 09/12] see if this works --- st2common/st2common/util/schema/__init__.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/util/schema/__init__.py b/st2common/st2common/util/schema/__init__.py index f6b4103b30..2b8f118f33 100644 --- a/st2common/st2common/util/schema/__init__.py +++ b/st2common/st2common/util/schema/__init__.py @@ -419,9 +419,16 @@ def validate( :type use_default: ``bool`` """ instance = fast_deepcopy_dict(instance) - schema_type = schema.get("type", None) + schema_type = None + if isinstance(schema, dict): + schema_type = schema.get("type", None) + instance_is_dict = isinstance(instance, dict) + if isinstance(schema, dict) and ("type" in schema or "$schema" in schema): + jsonschema.validate(instance=instance, schema=schema, *args, **kwargs) + return instance + if use_default and allow_default_none: schema = modify_schema_allow_default_none(schema=schema) From 994f1f75ddc9de7ff27ad96a140a066220a03bfd Mon Sep 17 00:00:00 2001 From: Robert Browning Date: Tue, 14 Oct 2025 11:17:41 -0700 Subject: [PATCH 10/12] Revert "see if this works" This reverts commit 89d6cdc4146468efceebbb5afdbc18b3a7fd9486. --- st2common/st2common/util/schema/__init__.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/st2common/st2common/util/schema/__init__.py b/st2common/st2common/util/schema/__init__.py index 2b8f118f33..f6b4103b30 100644 --- a/st2common/st2common/util/schema/__init__.py +++ b/st2common/st2common/util/schema/__init__.py @@ -419,16 +419,9 @@ def validate( :type use_default: ``bool`` """ instance = fast_deepcopy_dict(instance) - schema_type = None - if isinstance(schema, dict): - schema_type = schema.get("type", None) - + schema_type = schema.get("type", None) instance_is_dict = isinstance(instance, dict) - if isinstance(schema, dict) and ("type" in schema or "$schema" in schema): - jsonschema.validate(instance=instance, schema=schema, *args, **kwargs) - return instance - if use_default and allow_default_none: schema = modify_schema_allow_default_none(schema=schema) From b58413184c0e5edad9ec8b1b36205a5b3615f6ac Mon Sep 17 00:00:00 2001 From: Robert Browning Date: Tue, 14 Oct 2025 13:17:34 -0700 Subject: [PATCH 11/12] add the other types --- .../runners/local_runner/local_runner/runner.yaml | 10 ++++++++-- .../remote_runner/remote_runner/runner.yaml | 10 ++++++++-- .../runners/winrm_runner/winrm_runner/runner.yaml | 15 ++++++++++++--- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/contrib/runners/local_runner/local_runner/runner.yaml b/contrib/runners/local_runner/local_runner/runner.yaml index f2eec20966..f6d97b835b 100644 --- a/contrib/runners/local_runner/local_runner/runner.yaml +++ b/contrib/runners/local_runner/local_runner/runner.yaml @@ -45,8 +45,11 @@ anyOf: - type: "object" - type: "string" - - type: "array" + - type: "integer" - type: "number" + - type: "boolean" + - type: "array" + - type: "null" stderr: type: string return_code: @@ -104,8 +107,11 @@ anyOf: - type: "object" - type: "string" - - type: "array" + - type: "integer" - type: "number" + - type: "boolean" + - type: "array" + - type: "null" stderr: type: string return_code: diff --git a/contrib/runners/remote_runner/remote_runner/runner.yaml b/contrib/runners/remote_runner/remote_runner/runner.yaml index 807ec27d67..414a5b3582 100644 --- a/contrib/runners/remote_runner/remote_runner/runner.yaml +++ b/contrib/runners/remote_runner/remote_runner/runner.yaml @@ -94,8 +94,11 @@ anyOf: - type: "object" - type: "string" - - type: "array" + - type: "integer" - type: "number" + - type: "boolean" + - type: "array" + - type: "null" stderr: type: string return_code: @@ -192,8 +195,11 @@ anyOf: - type: "object" - type: "string" - - type: "array" + - type: "integer" - type: "number" + - type: "boolean" + - type: "array" + - type: "null" stderr: type: string return_code: diff --git a/contrib/runners/winrm_runner/winrm_runner/runner.yaml b/contrib/runners/winrm_runner/winrm_runner/runner.yaml index 5727c9457b..6e5a9b94e4 100644 --- a/contrib/runners/winrm_runner/winrm_runner/runner.yaml +++ b/contrib/runners/winrm_runner/winrm_runner/runner.yaml @@ -78,8 +78,11 @@ anyOf: - type: "object" - type: "string" - - type: "array" + - type: "integer" - type: "number" + - type: "boolean" + - type: "array" + - type: "null" stderr: type: string return_code: @@ -164,8 +167,11 @@ anyOf: - type: "object" - type: "string" - - type: "array" + - type: "integer" - type: "number" + - type: "boolean" + - type: "array" + - type: "null" stderr: type: string return_code: @@ -247,8 +253,11 @@ anyOf: - type: "object" - type: "string" - - type: "array" + - type: "integer" - type: "number" + - type: "boolean" + - type: "array" + - type: "null" stderr: type: string return_code: From 9b4bf7e4b416728bffd608a92bf3dc9d239b6b81 Mon Sep 17 00:00:00 2001 From: Robert Browning Date: Tue, 14 Oct 2025 13:38:05 -0700 Subject: [PATCH 12/12] see if we can use this to correctly validate some schemas --- st2common/st2common/util/schema/__init__.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/st2common/st2common/util/schema/__init__.py b/st2common/st2common/util/schema/__init__.py index f6b4103b30..2b8f118f33 100644 --- a/st2common/st2common/util/schema/__init__.py +++ b/st2common/st2common/util/schema/__init__.py @@ -419,9 +419,16 @@ def validate( :type use_default: ``bool`` """ instance = fast_deepcopy_dict(instance) - schema_type = schema.get("type", None) + schema_type = None + if isinstance(schema, dict): + schema_type = schema.get("type", None) + instance_is_dict = isinstance(instance, dict) + if isinstance(schema, dict) and ("type" in schema or "$schema" in schema): + jsonschema.validate(instance=instance, schema=schema, *args, **kwargs) + return instance + if use_default and allow_default_none: schema = modify_schema_allow_default_none(schema=schema)