Action delete API rework - support for action file deletion

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 <pack>.<action>`` will delete only action database entry.
+  From CLI ``st2 action delete --remove-files <pack>.<action>`` or ``st2 action delete -r <pack>.<action>``
+  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.
 

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)

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,
+        )

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)

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)

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):

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:

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: