CBG-5065 sgcollect: remove tmpdir #7941
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a regression in temporary directory cleanup for the sgcollect tool by implementing a context manager pattern for TaskRunner. The key changes ensure that temporary directories are properly cleaned up in all execution paths, including exceptions and SIGTERM signals.
Key Changes:
- Converted
TaskRunnerto a context manager with__enter__and__exit__methods for automatic cleanup - Restructured exception handling in the main flow to ensure zip files are deleted even if zipping or upload operations fail
- Added comprehensive test coverage to verify temporary directory cleanup across various scenarios
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tools/tasks.py | Added context manager implementation to TaskRunner class with cleanup in __exit__ |
| tools/sgcollect.py | Changed TaskRunner instantiation to use context manager pattern and restructured exception handling |
| tools-tests/upload_test.py | Added test fixture for temporary workdir and assertions to verify cleanup in all test cases |
| exit_code = do_upload(zip_filename, upload_url, options.upload_proxy) | ||
| sys.exit(exit_code) | ||
| finally: | ||
| if not options.keep_zip and upload_url: |
There was a problem hiding this comment.
The condition not options.keep_zip and upload_url will prevent zip deletion when upload_url is empty/None, even without the --keep-zip flag. This changes the behavior: previously, zips were deleted in the finally block regardless of upload_url when --keep-zip was not set. Now they're only deleted when an upload URL exists. This could leave zip files behind when no upload is performed. Consider if this behavior change is intended or if the condition should be if not options.keep_zip:.
| if not options.keep_zip and upload_url: | |
| if not options.keep_zip: |
CBG-5065 sgcollect: remove tmpdir
atexithandling. Using a context manager is more robust thanatexitsince it will cover SIGTERM exiting as well.finallystatement earlier if it fails inrunner.redact_and_ziporrunner.zipMost changes are whitespace only to indent.
Pre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/api