diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 478156beb4..676df63b7d 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -7,12 +7,16 @@ in development Changed ~~~~~~~ -* Modified action delete api. Action delete api removes related action/workflow files on disk - along with de-registering them from database. Prompts on CLI for user permission before - removing disk files. +* Modified action delete API to delete action files from disk along with backward compatibility. - ``-f`` and ``--force`` arguments added for action delete CLI command as auto yes flag and - will delete related files on disk without prompting for user permission. #5304 + From CLI ``st2 action delete .`` will delete only action database entry. + From CLI ``st2 action delete --remove-files .`` or ``st2 action delete -r .`` + will delete action database entry along with files from disk. + + API action DELETE method with ``{"remove_files": true}`` argument in json body will remove database + entry of action along with files from disk. + API action DELETE method with ``{"remove_files": false}`` or no additional argument in json body will remove + only action database entry. #5304, #5351, #5360 Contributed by @mahesh-orch. diff --git a/st2api/st2api/controllers/v1/actions.py b/st2api/st2api/controllers/v1/actions.py index d1ee3d88b8..88e5993e49 100644 --- a/st2api/st2api/controllers/v1/actions.py +++ b/st2api/st2api/controllers/v1/actions.py @@ -206,7 +206,7 @@ def put(self, action, ref_or_id, requester_user): return action_api - def delete(self, ref_or_id, requester_user): + def delete(self, options, ref_or_id, requester_user): """ Delete an action. @@ -252,30 +252,30 @@ def delete(self, ref_or_id, requester_user): ) abort(http_client.INTERNAL_SERVER_ERROR, six.text_type(e)) return - try: - delete_action_files_from_pack( - pack_name=pack_name, - entry_point=entry_point, - metadata_file=metadata_file, - ) - - except PermissionError as e: - LOG.error("No permission to delete resource files from disk.") - action_db.id = None - Action.add_or_update(action_db) - abort(http_client.FORBIDDEN, six.text_type(e)) - return - except Exception as e: - LOG.error( - "Exception encountered during deleting resource files from disk. " - "Exception was %s", - e, - ) - action_db.id = None - Action.add_or_update(action_db) - abort(http_client.INTERNAL_SERVER_ERROR, six.text_type(e)) - return + if options.remove_files: + try: + delete_action_files_from_pack( + pack_name=pack_name, + entry_point=entry_point, + metadata_file=metadata_file, + ) + except PermissionError as e: + LOG.error("No permission to delete resource files from disk.") + action_db.id = None + Action.add_or_update(action_db) + abort(http_client.FORBIDDEN, six.text_type(e)) + return + except Exception as e: + LOG.error( + "Exception encountered during deleting resource files from disk. " + "Exception was %s", + e, + ) + action_db.id = None + Action.add_or_update(action_db) + abort(http_client.INTERNAL_SERVER_ERROR, six.text_type(e)) + return extra = {"action_db": action_db} LOG.audit("Action deleted. Action.id=%s" % (action_db.id), extra=extra) diff --git a/st2api/tests/unit/controllers/v1/test_actions.py b/st2api/tests/unit/controllers/v1/test_actions.py index af6ad3d91d..f28db5d52f 100644 --- a/st2api/tests/unit/controllers/v1/test_actions.py +++ b/st2api/tests/unit/controllers/v1/test_actions.py @@ -618,13 +618,54 @@ def test_post_override_runner_param_allowed(self): post_resp = self.__do_post(ACTION_15) self.assertEqual(post_resp.status_int, 201) + @mock.patch("st2api.controllers.v1.actions.delete_action_files_from_pack") @mock.patch.object( action_validator, "validate_action", mock.MagicMock(return_value=True) ) - def test_delete(self): + def test_delete(self, mock_remove_files): post_resp = self.__do_post(ACTION_1) - del_resp = self.__do_delete(self.__get_action_id(post_resp)) + action_id = self.__get_action_id(post_resp) + del_resp = self.__do_delete(action_id) + self.assertEqual(del_resp.status_int, 204) + mock_remove_files.assert_not_called() + + # asserting ACTION_1 database entry has removed + get_resp = self.__do_get_one(action_id, expect_errors=True) + expected_msg = 'Resource with a reference or id "%s" not found' % action_id + actual_msg = get_resp.json["faultstring"] + self.assertEqual(actual_msg, expected_msg) + + @mock.patch("st2api.controllers.v1.actions.delete_action_files_from_pack") + @mock.patch.object( + action_validator, "validate_action", mock.MagicMock(return_value=True) + ) + def test_delete_remove_files_false(self, mock_remove_files): + post_resp = self.__do_post(ACTION_1) + action_id = self.__get_action_id(post_resp) + payload = {"remove_files": False} + del_resp = self.__do_delete_action_with_files(payload, action_id) self.assertEqual(del_resp.status_int, 204) + mock_remove_files.assert_not_called() + get_resp = self.__do_get_one(action_id, expect_errors=True) + expected_msg = 'Resource with a reference or id "%s" not found' % action_id + actual_msg = get_resp.json["faultstring"] + self.assertEqual(actual_msg, expected_msg) + + @mock.patch("st2api.controllers.v1.actions.delete_action_files_from_pack") + @mock.patch.object( + action_validator, "validate_action", mock.MagicMock(return_value=True) + ) + def test_delete_remove_files_true(self, mock_remove_files): + post_resp = self.__do_post(ACTION_1) + action_id = self.__get_action_id(post_resp) + payload = {"remove_files": True} + del_resp = self.__do_delete_action_with_files(payload, action_id) + self.assertEqual(del_resp.status_int, 204) + self.assertTrue(mock_remove_files.called) + get_resp = self.__do_get_one(action_id, expect_errors=True) + expected_msg = 'Resource with a reference or id "%s" not found' % action_id + actual_msg = get_resp.json["faultstring"] + self.assertEqual(actual_msg, expected_msg) @mock.patch.object( action_validator, "validate_action", mock.MagicMock(return_value=True) @@ -637,8 +678,9 @@ def test_delete_permission_error_and_action_reregistered_to_database(self): ) as mock_remove_files: msg = "No permission to delete action files from disk" mock_remove_files.side_effect = PermissionError(msg) - del_resp = self.__do_delete( - self.__get_action_id(post_resp), expect_errors=True + payload = {"remove_files": True} + del_resp = self.__do_delete_action_with_files( + payload, self.__get_action_id(post_resp), expect_errors=True ) self.assertEqual(del_resp.status_int, 403) self.assertEqual(del_resp.json["faultstring"], msg) @@ -663,8 +705,9 @@ def test_delete_exception_and_action_reregistered_to_database(self): ) as mock_remove_files: msg = "Exception encountered during removing files from disk" mock_remove_files.side_effect = Exception(msg) - del_resp = self.__do_delete( - self.__get_action_id(post_resp), expect_errors=True + payload = {"remove_files": True} + del_resp = self.__do_delete_action_with_files( + payload, self.__get_action_id(post_resp), expect_errors=True ) self.assertEqual(del_resp.status_int, 500) self.assertEqual(del_resp.json["faultstring"], msg) @@ -854,3 +897,10 @@ def __do_delete(self, action_id, expect_errors=False): return self.app.delete( "/v1/actions/%s" % action_id, expect_errors=expect_errors ) + + def __do_delete_action_with_files(self, options, action_id, expect_errors=False): + return self.app.delete_json( + "/v1/actions/%s" % action_id, + options, + expect_errors=expect_errors, + ) diff --git a/st2client/st2client/commands/action.py b/st2client/st2client/commands/action.py index 76b7e61203..41fca2da88 100644 --- a/st2client/st2client/commands/action.py +++ b/st2client/st2client/commands/action.py @@ -282,31 +282,21 @@ def __init__(self, resource, *args, **kwargs): super(ActionDeleteCommand, self).__init__(resource, *args, **kwargs) self.parser.add_argument( - "-f", - "--force", + "-r", + "--remove-files", action="store_true", - dest="force", - help="Auto yes flag to delete action files from disk.", + dest="remove_files", + default=False, + help="Remove action files from disk.", ) @add_auth_token_to_kwargs_from_cli def run(self, args, **kwargs): resource_id = getattr(args, self.pk_argument_name, None) instance = self.get_resource(resource_id, **kwargs) - msg = ( - 'Resource with id "%s" has been successfully deleted from database and disk.' - % (resource_id) - ) - user_input = "" - if not args.force: - user_input = input( - "The resource files on disk will be deleted. Do you want to continue? (y/n): " - ) - if args.force or user_input.lower() == "y" or user_input.lower() == "yes": - self.manager.delete(instance, **kwargs) - print(msg) - else: - print("Action is not deleted.") + remove_files = args.remove_files + self.manager.delete_action(instance, remove_files, **kwargs) + print('Resource with id "%s" has been successfully deleted.' % (resource_id)) def run_and_print(self, args, **kwargs): resource_id = getattr(args, self.pk_argument_name) diff --git a/st2client/st2client/models/core.py b/st2client/st2client/models/core.py index 7f94cc94f8..45034b6c6b 100644 --- a/st2client/st2client/models/core.py +++ b/st2client/st2client/models/core.py @@ -376,6 +376,21 @@ def delete(self, instance, **kwargs): return True + @add_auth_token_to_kwargs_from_env + def delete_action(self, instance, remove_files, **kwargs): + url = "/%s/%s" % (self.resource.get_url_path_name(), instance.id) + payload = {"remove_files": remove_files} + response = self.client.delete(url, data=orjson.dumps(payload), **kwargs) + if response.status_code not in [ + http_client.OK, + http_client.NO_CONTENT, + http_client.NOT_FOUND, + ]: + self.handle_error(response) + return False + + return True + @add_auth_token_to_kwargs_from_env def delete_by_id(self, instance_id, **kwargs): url = "/%s/%s" % (self.resource.get_url_path_name(), instance_id) diff --git a/st2client/tests/unit/test_models.py b/st2client/tests/unit/test_models.py index 8a137afa13..665942ddcb 100644 --- a/st2client/tests/unit/test_models.py +++ b/st2client/tests/unit/test_models.py @@ -355,6 +355,56 @@ def test_resource_delete_failed(self): instance = mgr.get_by_name("abc") self.assertRaises(Exception, mgr.delete, instance) + @mock.patch.object( + httpclient.HTTPClient, + "get", + mock.MagicMock( + return_value=base.FakeResponse( + json.dumps([base.RESOURCES[0]]), 200, "OK", {} + ) + ), + ) + @mock.patch.object( + httpclient.HTTPClient, + "delete", + mock.MagicMock(return_value=base.FakeResponse("", 204, "NO CONTENT")), + ) + def test_resource_delete_action(self): + mgr = models.ResourceManager(base.FakeResource, base.FAKE_ENDPOINT) + instance = mgr.get_by_name("abc") + mgr.delete_action(instance, True) + + @mock.patch.object( + httpclient.HTTPClient, + "delete", + mock.MagicMock(return_value=base.FakeResponse("", 404, "NOT FOUND")), + ) + def test_resource_delete_action_404(self): + mgr = models.ResourceManager(base.FakeResource, base.FAKE_ENDPOINT) + instance = base.FakeResource.deserialize(base.RESOURCES[0]) + mgr.delete_action(instance, False) + + @mock.patch.object( + httpclient.HTTPClient, + "get", + mock.MagicMock( + return_value=base.FakeResponse( + json.dumps([base.RESOURCES[0]]), 200, "OK", {} + ) + ), + ) + @mock.patch.object( + httpclient.HTTPClient, + "delete", + mock.MagicMock( + return_value=base.FakeResponse("", 500, "INTERNAL SERVER ERROR") + ), + ) + def test_resource_delete_action_failed(self): + mgr = models.ResourceManager(base.FakeResource, base.FAKE_ENDPOINT) + instance = mgr.get_by_name("abc") + self.assertRaises(Exception, mgr.delete_action, instance, True) + @mock.patch("requests.get") @mock.patch("sseclient.SSEClient") def test_stream_resource_listen(self, mock_sseclient, mock_requests): diff --git a/st2common/st2common/openapi.yaml b/st2common/st2common/openapi.yaml index 723c6f5235..0ca1430c34 100644 --- a/st2common/st2common/openapi.yaml +++ b/st2common/st2common/openapi.yaml @@ -413,6 +413,11 @@ paths: description: Entity reference or id type: string required: true + - name: options + in: body + description: Flag to remove action files from disk + schema: + $ref: '#/definitions/ActionDeleteRequest' x-parameters: - name: user in: context @@ -4684,6 +4689,9 @@ definitions: allOf: - $ref: '#/definitions/Action' - $ref: '#/definitions/DataFilesSubSchema' + ActionDeleteRequest: + allOf: + - $ref: '#/definitions/ActionDeleteSchema' ActionParameters: type: object properties: @@ -5384,6 +5392,14 @@ definitions: items: type: string additionalProperties: false + ActionDeleteSchema: + type: object + properties: + remove_files: + type: boolean + description: Flag to delete action files from disk + default: false + additionalProperties: false TokenRequest: type: object properties: diff --git a/st2common/st2common/openapi.yaml.j2 b/st2common/st2common/openapi.yaml.j2 index b41602587b..f2368cd74c 100644 --- a/st2common/st2common/openapi.yaml.j2 +++ b/st2common/st2common/openapi.yaml.j2 @@ -409,6 +409,11 @@ paths: description: Entity reference or id type: string required: true + - name: options + in: body + description: Flag to remove action files from disk + schema: + $ref: '#/definitions/ActionDeleteRequest' x-parameters: - name: user in: context @@ -4680,6 +4685,9 @@ definitions: allOf: - $ref: '#/definitions/Action' - $ref: '#/definitions/DataFilesSubSchema' + ActionDeleteRequest: + allOf: + - $ref: '#/definitions/ActionDeleteSchema' ActionParameters: type: object properties: @@ -5380,6 +5388,14 @@ definitions: items: type: string additionalProperties: false + ActionDeleteSchema: + type: object + properties: + remove_files: + type: boolean + description: Flag to delete action files from disk + default: false + additionalProperties: false TokenRequest: type: object properties: