-
-
Notifications
You must be signed in to change notification settings - Fork 782
Modify API to remove related files on delete of actions/workflows #5304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
28a3c39
addd291
b70b4a3
f6b91b9
ca4d453
e7b32b0
d86d136
7ca0f6c
0639640
c7354c7
b6ed390
7395008
284e130
f929dc5
ad6c868
bee06cc
54a70e6
953d6a1
8c37c5e
0769eb1
9b9b9d0
45a7fb9
bf538d0
ea3dac3
947a16c
4995144
e974022
d95d8b7
9e99615
4916c04
5c0eb6f
edce1d8
4da0f48
2654528
6179ca6
b4d7d5e
8743f82
d35a59e
bee5a3f
858b3b1
5439154
f003988
d8db49e
04bc3b2
ec307f9
6bd83fe
37a36de
dcbf567
89cbc60
b581fc7
24cbc0a
80788e8
749c187
ec59efd
13eae54
4a0bbdd
63f3d12
936f618
96029a0
5fe66c0
e76573c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,12 +17,15 @@ | |
|
|
||
| import itertools | ||
|
|
||
| import os | ||
| import requests | ||
| import six | ||
| from six.moves import range | ||
| from oslo_config import cfg | ||
|
|
||
| from st2common import log as logging | ||
| from st2common.content.utils import get_pack_base_path | ||
| from st2common.exceptions.content import ResourceDiskFilesRemovalError | ||
| from st2common.persistence.pack import Pack | ||
| from st2common.util.misc import lowercase_value | ||
| from st2common.util.jsonify import json_encode | ||
|
|
@@ -32,6 +35,7 @@ | |
| "fetch_pack_index", | ||
| "get_pack_from_index", | ||
| "search_pack_index", | ||
| "delete_action_files_from_pack", | ||
| ] | ||
|
|
||
| EXCLUDE_FIELDS = ["repo_url", "email"] | ||
|
|
@@ -215,3 +219,74 @@ def search_pack_index( | |
| break | ||
|
|
||
| return list(itertools.chain.from_iterable(matches)) | ||
|
|
||
|
|
||
| def delete_action_files_from_pack(pack_name, entry_point, metadata_file): | ||
| """ | ||
| Prepares the path for entry_point file and metadata file of action and | ||
| deletes them from disk. | ||
| """ | ||
|
|
||
| pack_base_path = get_pack_base_path(pack_name=pack_name) | ||
| action_entrypoint_file_path = os.path.join(pack_base_path, "actions", entry_point) | ||
| action_metadata_file_path = os.path.join(pack_base_path, metadata_file) | ||
|
|
||
| if os.path.isfile(action_entrypoint_file_path): | ||
| try: | ||
| os.remove(action_entrypoint_file_path) | ||
| except PermissionError: | ||
| LOG.error( | ||
| 'No permission to delete the "%s" file', | ||
| action_entrypoint_file_path, | ||
| ) | ||
| msg = 'No permission to delete "%s" file from disk' % ( | ||
| action_entrypoint_file_path | ||
| ) | ||
| raise PermissionError(msg) | ||
| except Exception as e: | ||
| LOG.error( | ||
| 'Unable to delete "%s" file. Exception was "%s"', | ||
| action_entrypoint_file_path, | ||
| e, | ||
| ) | ||
| msg = ( | ||
| 'The action file "%s" could not be removed from disk, please ' | ||
| "check the logs or ask your StackStorm administrator to check " | ||
| "and delete the actions files manually" % (action_entrypoint_file_path) | ||
| ) | ||
| raise ResourceDiskFilesRemovalError(msg) | ||
| else: | ||
| LOG.warning( | ||
| 'The action entry point file "%s" does not exists on disk.', | ||
| action_entrypoint_file_path, | ||
| ) | ||
|
|
||
| if os.path.isfile(action_metadata_file_path): | ||
| try: | ||
| os.remove(action_metadata_file_path) | ||
| except PermissionError: | ||
| LOG.error( | ||
| 'No permission to delete the "%s" file', | ||
| action_metadata_file_path, | ||
| ) | ||
| msg = 'No permission to delete "%s" file from disk' % ( | ||
| action_metadata_file_path | ||
| ) | ||
| raise PermissionError(msg) | ||
| except Exception as e: | ||
| LOG.error( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we fail to delete the files from disk as we are catching the exceptions, does this not mean that the API is going to return success? Perhaps re-throw an exception if the file still exists after the delete failed? So that in the case @blag was saying about two workers trying to delete and file doesn't exist, it would still return success. But if it failed to delete the file then it would inform the user. This would allow in the api/controllers you can then return an internal server error if the file still exists after you attempted to delete it. |
||
| 'Could not delete "%s" file. Exception was "%s"', | ||
| action_metadata_file_path, | ||
| e, | ||
| ) | ||
| msg = ( | ||
| 'The action file "%s" could not be removed from disk, please ' | ||
| "check the logs or ask your StackStorm administrator to check " | ||
| "and delete the actions files manually" % (action_metadata_file_path) | ||
| ) | ||
| raise ResourceDiskFilesRemovalError(msg) | ||
| else: | ||
| LOG.warning( | ||
| 'The action metadata file "%s" does not exists on disk.', | ||
| action_metadata_file_path, | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the Action.delete and delete_action_files_from disk are in the same catch exception. If I get an error on deleting the files but not on the database, doesn't this mean that I'm still going to get a LOG.error saying "Database delete encountered exception"?
This would be a bit confusing. So either we need to amend the text slightly to indicate it might be database or file, or have two catch blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mahesh-orch Please address this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you put the try/except blocks one after the other instead of nesting them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! that looks much better.