Feature/cleanup script - #5
Conversation
… specific folders with a retention period. Deleting folders and files which their last modified date is older than the retention period.
…only file removal
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| def cleanup(dir_path, retention_days): | ||
| if not os.path.exists(dir_path): | ||
| print(f"Path does not exist: {dir_path}") | ||
| return | ||
|
|
||
| if not os.path.isdir(dir_path): | ||
| print(f"Path is not a directory: {dir_path}") | ||
| return | ||
|
|
||
| for entry in os.listdir(dir_path): | ||
| entry_path = os.path.join(dir_path, entry) | ||
| handle_path(entry_path, retention_days) | ||
|
|
||
| if not os.listdir(dir_path): | ||
| handle_directory_removal(dir_path) |
There was a problem hiding this comment.
This does not check for symlinks (on the entry level). Does the caller know or can be trusted with these things?
| for entry in contents: | ||
| entry_path = os.path.join(directory, entry) | ||
| handle_path(entry_path, retention_days) |
There was a problem hiding this comment.
This recurses into other directories, but does not check for symlinks. A mistake could therefore delete folders outside of the cache dirs, making it possible to (depending on the permissions the user has) delete stuff that is system critical. This could throw an error when a symlink is encountered (os.path.islink(entry_path) if claude is to be believed).
There was a problem hiding this comment.
The cleanup function itself is not covered by tests, only the helper functions.
| if func in (os.rmdir, os.remove, os.unlink) and excvalue.errno == errno.EACCES: | ||
| # Windows marks some OneDrive files read-only, which makes rmdir/remove raise | ||
| # "access denied" (EACCES). Clearing the attribute and retrying fixes that case. | ||
| os.chmod(path, stat.S_IRWXU| stat.S_IRWXG| stat.S_IRWXO) # 0777 | ||
| func(path) | ||
| else: | ||
| # Not a read-only issue (e.g. an ACL delete-deny) - re-raise the original exception. | ||
| # Works because rmtree calls onerror() from inside its own except block, so the | ||
| # exception context is still live for a bare `raise` to pick up. | ||
| raise |
There was a problem hiding this comment.
indentation levels drop back to 2 spaces instead of 4 for the rest of the file.
| if last_modified_days <= retention_days: | ||
| return |
There was a problem hiding this comment.
Is there a use-case for debug logging output on these checks?
| if func in (os.rmdir, os.remove, os.unlink) and excvalue.errno == errno.EACCES: | ||
| # Windows marks some OneDrive files read-only, which makes rmdir/remove raise | ||
| # "access denied" (EACCES). Clearing the attribute and retrying fixes that case. | ||
| os.chmod(path, stat.S_IRWXU| stat.S_IRWXG| stat.S_IRWXO) # 0777 |
There was a problem hiding this comment.
Sonar's comment is valid, and I wonder if we need this type of feature to cater for windows use? @agjharms your opinion on this?
There was a problem hiding this comment.
As far as I know, neither at Health-RI, nor at any UMC, XNAT is run on a Windows environment and the kind of folders that this cleanup script is meant for will not be placed on a OneDrive folder. I appreciate the inclusion of this environment but in my opinion we can drop the support for this.
|
|
||
| try: | ||
| os.remove(file_path) | ||
| print(f"Removed file {file_path} - last modified {last_modified_days} days") |
There was a problem hiding this comment.
Sonar's comment is reasonable, to a certain extend. If this is only ever ran by a human who knows what they are doing this is an acceptable risk. Still we could add extra checks for whether the user under which the script runs has the right permissions, and if so check whether it's not a system folder?
There was a problem hiding this comment.
This script will only be run, or set up to be run, by administrators of XNAT deployments. I don't see any reason why this script would be run by an LLM, so we don't need to secure ourselves against that scenario.
…ion period, add seperate function for checking if directory is empty.
|
4 participants
Summary
Adds a
folder_cleanup.pyscript for removing old files/folders from a provided path (e.g. cache, temp, deleted) based on a retention period provided.--dir_path) and removes files whose last-modified time exceeds--retention_days(default: 90)EACCESerrorsthe attribute and retryingTest plan
handle_path,handle_directory,handle_file_removal, and readonly-file/directory removal (tests/test_folder_cleanup.py)