Skip to content

Conversation

@leander-dsouza
Copy link
Member

Basic Info

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

Description of contribution in a few bullet points

  • Added a delete command for vcs to remove all the paths specified under the YAML repository file.
  • By default, the following command prints information about the YAML file, but does not delete until a -f/--force option is passed:
    $ vcs delete --input list.repos .
    Warning: The following paths do not exist:
      ./immutable/hash
      ./immutable/hash_tar
      ./immutable/hash_zip
      ./immutable/tag
      ./without_version
    The following paths will be deleted:
      ./vcs2l
    Dry-run mode: No directories were deleted. Use -f/--force to actually delete them.
  • Updated the testing framework to account for various edge cases regarding the delete feature.
  • Updated the README to include information on how to use the new command.

Description of how this change was tested

  • Ran pytest locally to ensure all the unit and linting tests pass:

    pytest -s -v test
  • Created a local fork to validate green CI.

Signed-off-by: Leander Stephen Desouza [email protected]

@leander-dsouza leander-dsouza force-pushed the leander-dsouza/add-deletion-option branch 4 times, most recently from 0901562 to f0ca719 Compare August 25, 2025 08:03
@leander-dsouza leander-dsouza marked this pull request as ready for review August 25, 2025 08:05
Copy link
Contributor

@claraberendsen claraberendsen left a comment

Choose a reason for hiding this comment

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

Left some nitpicks in terms of unifying naming towards directories or paths when given messages to the user.
Overall I think it looks good to me.
I think we should add a test-guard that checks that only the folders specified in the yaml are being deleted since users might have other stuff co-living with repositories managed by vcs.

Copy link
Contributor

Choose a reason for hiding this comment

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

What would happen if the YAML is empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

You would get the following error:

Error: Failed to read repositories: Input data is not valid format: 'NoneType' object is not subscriptable

This is captured from the following line in delete.py.

@leander-dsouza leander-dsouza force-pushed the leander-dsouza/add-deletion-option branch 2 times, most recently from 973de9b to 093a26a Compare August 28, 2025 13:01
@leander-dsouza leander-dsouza force-pushed the leander-dsouza/add-deletion-option branch from 093a26a to 7daff24 Compare August 28, 2025 13:06
@leander-dsouza
Copy link
Member Author

I think we should add a test-guard that checks that only the folders specified in the yaml are being deleted since users might have other stuff co-living with repositories managed by vcs.

I have added a few assertions in the integration test to verify the specific deletion of folders over here.

@leander-dsouza leander-dsouza merged commit 2b528f2 into main Aug 29, 2025
14 checks passed
@leander-dsouza leander-dsouza deleted the leander-dsouza/add-deletion-option branch August 29, 2025 14:57
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