[4.0.2.1 backport] CBG-5067 sgcollect: remove tmpdir #7944
Conversation
cherry-pick of 00cbdeb Fix regression and remove tmpdir and zipfiles via context manager
There was a problem hiding this comment.
Pull request overview
This PR implements a regression fix by converting TaskRunner to a context manager that properly cleans up temporary directories and zip files after use. The change ensures tmpdir and associated resources are removed automatically when TaskRunner goes out of scope, addressing resource cleanup issues.
Key Changes
TaskRunnernow implements context manager protocol (__enter__and__exit__)- Temporary directory cleanup moved from manual to automatic via
__exit__ - Zip file deletion logic modified to only execute when upload URL is present
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/tasks.py | Adds context manager support to TaskRunner, implementing __enter__ and __exit__ methods for automatic cleanup |
| tools/sgcollect.py | Wraps TaskRunner instantiation in a with statement and adjusts zip deletion logic to check for upload URL |
| tools-tests/upload_test.py | Adds taskrunner_workdir fixture and verifies temporary directory cleanup in all test cases |
| def __exit__( | ||
| self, | ||
| exc_type: Optional[type[BaseException]], | ||
| exc_value: Optional[type[BaseException]], |
There was a problem hiding this comment.
The type annotation for exc_value is incorrect. It should be Optional[BaseException] (the exception instance) rather than Optional[type[BaseException]] (the exception type). The exc_type parameter is correctly typed as the exception type, but exc_value should be the actual exception instance.
| exc_value: Optional[type[BaseException]], | |
| exc_value: Optional[BaseException], |
| # Build the actual zip file | ||
| runner.zip(zip_filename, "sgcollect_info", platform.node()) |
There was a problem hiding this comment.
The try block begins immediately before runner.zip(), but since runner.close_all_files() was already called at line 948 (before the try), any exception during zip creation will result in the context manager's __exit__ attempting to close already-closed files. Consider restructuring to avoid double-closure or ensure close_all_files() is idempotent.
[4.0.2.1 backport] CBG-5067 sgcollect: remove tmpdir
cherry-pick of 00cbdeb
Fix regression and remove tmpdir and zipfiles via context manager