Skip to content

fix: resolve ResourceWarning from unclosed file handles in validate and import (fixes #116) - #121

Open
KmoM88 wants to merge 2 commits into
ros-infrastructure:mainfrom
KmoM88:fix/resource-warning-FileType
Open

KmoM88 wants to merge 2 commits into
ros-infrastructure:mainfrom
KmoM88:fix/resource-warning-FileType

Conversation

@KmoM88

@KmoM88 KmoM88 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Basic Info

The commands vcs validate and vcs import used argparse.FileType('r') which opens files at parse-time but doesn't provide a way to close them, resulting in unclosed file handle warnings.

This PR replaces argparse.FileType with string path arguments and explicitly manages the file descriptors using context managers for files, standard input, and URL streams.

Info Please fill out this column
Ticket(s) this addresses resolves #116
Primary OS tested on Ubuntu
Is this a breaking change? No
Does this PR contain AI generated software? Yes

Description of contribution in a few bullet points

  • Replaced argparse.FileType with string arguments: Avoids opening files at argument parse time, preventing automatic file handle leaks.
  • Implemented Context Managers: Ensures that local input files and remote URL streams are closed deterministically immediately after they are read.
  • Maintained Backwards Compatibility: Added explicit handling for stdin (-) to preserve standard pipe-input workflows without closing the system's stdin stream.

Description of how this change was tested

  1. Clone repository and create a Test File:

    • Create a file named example.repos with the following content:
      repositories:
        foo:
          type: git
          url: https://example.com/foo.git
          version: main
  2. Reproduce the Warning (on main branch):

    • Run the validate command with warnings enabled:
      PYTHONWARNINGS=default vcs validate --input example.repos --retry 0
    • You should see a ResourceWarning: unclosed file in the output.
  3. Verify the Fix (on fix/resource-warning-FileType branch):

    • Run the same command again:
      PYTHONWARNINGS=default vcs validate --input example.repos --retry 0
    • Expected: The command runs without any ResourceWarning.
    • Verify with the import command and with stdin as well:
      PYTHONWARNINGS=default vcs import --input example.repos --retry 0
      cat example.repos | PYTHONWARNINGS=default vcs validate --input - --retry 0
      cat example.repos | PYTHONWARNINGS=default vcs import --input - --retry 0
    • Expected: No warnings are shown for any of the commands.

…nd import (fixes ros-infrastructure#116)

The commands `vcs validate` and `vcs import` used `argparse.FileType('r')`
which opens files at parse-time but doesn't provide a way to close them,
resulting in unclosed file handle warnings.

This replaces `argparse.FileType` with string path arguments and explicitly
manages the file descriptors using context managers (`with` statements)
for files, standard input, and URL streams.

Signed-off-by: Federico Rossi <federico.rossi@creativa77.com.ar>
@KmoM88
KmoM88 marked this pull request as draft August 13, 2026 14:15
@codecov-commenter

codecov-commenter commented Aug 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 40.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.46%. Comparing base (fbd0321) to head (b8cff21).

Files with missing lines Patch % Lines
vcs2l/commands/import_.py 33.33% 5 Missing and 1 partial ⚠️
vcs2l/commands/validate.py 50.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #121       +/-   ##
===========================================
+ Coverage   26.76%   37.46%   +10.70%     
===========================================
  Files          31       31               
  Lines        2298     2306        +8     
  Branches      405      407        +2     
===========================================
+ Hits          615      864      +249     
+ Misses       1623     1334      -289     
- Partials       60      108       +48     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@KmoM88
KmoM88 force-pushed the fix/resource-warning-FileType branch 5 times, most recently from a6f5e33 to 7e37a4f Compare August 13, 2026 16:41
Signed-off-by: Federico Rossi <federico.rossi@creativa77.com.ar>
@KmoM88
KmoM88 marked this pull request as ready for review August 13, 2026 18:11
@leander-dsouza
leander-dsouza requested a balanced review from Copilot August 20, 2026 17:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Prevents file-handle leaks when validating or importing repository definitions.

Changes:

  • Defers opening input paths until command execution.
  • Closes local files and URL streams with context managers.
  • Adds direct command invocation tests.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
vcs2l/commands/validate.py Manages validation input streams explicitly.
vcs2l/commands/import_.py Manages file and URL input streams explicitly.
test/test_commands.py Adds direct invocation coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

else:
with open(args.input, 'r', encoding='utf-8') as f:
repos = get_repositories(f)
except (RuntimeError, FileNotFoundError) as e:
Comment thread vcs2l/commands/import_.py
else:
with open(input_, 'r', encoding='utf-8') as f:
repos = get_repositories(f)
except (RuntimeError, request.URLError, FileNotFoundError) as e:
Comment thread test/test_commands.py
Comment on lines +579 to +583
args=['--input', self.repos_file_path],
stdout=self.stdout,
stderr=self.stderr,
)
self.assertEqual(rc, 0)
Comment thread test/test_commands.py
unittest.main()


class TestDirectMainCall(StagedReposFile):
@leander-dsouza leander-dsouza added the bug Something isn't working label Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

validate/import: --input via argparse.FileType leaks an unclosed file (ResourceWarning)

4 participants