Skip to content

Conversation

@torcolvin
Copy link
Collaborator

[3.3.2.2 backport] CBG-5066 sgcollect: remove tmpdir

cherry-pick of 00cbdeb

Fix regression and remove tmpdir and zipfiles via context manager

cherry-pick of 00cbdeb

Fix regression and remove tmpdir and zipfiles via context manager
Copilot AI review requested due to automatic review settings January 6, 2026 14:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR backports a fix for CBG-5066 that addresses temporary directory cleanup in sgcollect by implementing a context manager pattern for the TaskRunner class. The change ensures that both the temporary directory and zip files are properly cleaned up after collection operations complete.

Key changes:

  • TaskRunner now implements context manager protocol to automatically cleanup temp directories
  • Zip file deletion logic now only triggers when upload URL is provided
  • Test coverage enhanced to verify temp directory cleanup

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 methods (__enter__/__exit__) to TaskRunner class for automatic cleanup of temporary directories and adds shutil import
tools/sgcollect.py Converts TaskRunner usage to context manager pattern and moves zip deletion logic inside try-finally block with conditional check for upload URL
tools-tests/upload_test.py Adds taskrunner_workdir fixture and assertions to verify temp directory cleanup across all test cases

def __exit__(
self,
exc_type: Optional[type[BaseException]],
exc_value: Optional[type[BaseException]],
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type annotation for exc_value parameter should be Optional[BaseException] not Optional[type[BaseException]]. The exception value is an instance, not a type.

Suggested change
exc_value: Optional[type[BaseException]],
exc_value: Optional[BaseException],

Copilot uses AI. Check for mistakes.
Comment on lines 144 to 145
with unittest.mock.patch(
"sys.argv",
Copy link

Copilot AI Jan 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing --tmp-dir argument in test mock for consistency. While the test may pass without it, all other upload tests include this argument to ensure the temporary directory is tracked and cleaned up properly.

Copilot uses AI. Check for mistakes.
@bbrks bbrks merged commit 49c1772 into release/3.3.2.2 Jan 6, 2026
44 checks passed
@bbrks bbrks deleted the CBG-5066 branch January 6, 2026 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants