delete-project subcommand - #9
Conversation
This is some cleanup of older code
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9 +/- ##
==========================================
+ Coverage 99.70% 99.73% +0.03%
==========================================
Files 13 13
Lines 2405 2690 +285
==========================================
+ Hits 2398 2683 +285
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds a new delete-project subcommand to the DevBox CLI and refactors the terminate_instance method to use structured return values and exceptions instead of tuple-based success/failure indicators.
Changes:
- Adds
delete-projectcommand with two-stage confirmation (database entry deletion, then AMI/snapshot cleanup) and--forceflag to skip confirmations - Refactors error handling pattern from returning
(bool, str)tuples to raising exceptions and returning structured dictionaries - Adds four new helper methods to DevBoxManager:
get_project_item,project_in_use,delete_project_entry, anddelete_ami_and_snapshots
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| src/devbox/devbox_manager.py | Adds helper methods for project deletion and refactors terminate_instance to raise exceptions instead of returning status tuples |
| src/devbox/cli.py | Implements delete-project command with two-stage confirmation and updates terminate command to handle new return format |
| tests/test_devbox_manager.py | Updates terminate_instance tests to expect exceptions and adds basic tests for new helper methods |
| tests/test_cli.py | Updates terminate tests and adds comprehensive test suite for delete-project command covering success and cancellation scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except ClientError as e: | ||
| error_code = e.response.get('Error', {}).get('Code', 'UnknownError') | ||
| raise utils.AWSClientError( | ||
| f"Error deleting AMI {ami_id}: {error_code} - {str(e)}", | ||
| error_code=error_code, | ||
| original_exception=e | ||
| ) |
There was a problem hiding this comment.
The error message "Error deleting AMI" at line 326 is misleading when the error occurs during the describe_images call (line 293) before any deletion has occurred. Consider separating the error handling or making the error message more generic, such as "Error processing AMI deletion" or handling describe_images errors separately with a message like "Error retrieving AMI information".
There was a problem hiding this comment.
The additional error information, from the original error, should be sufficient to clarify.
Also updated some old stuff to use unittest.mock.patch instead of manually patching.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This prevents the AMI from being orphaned if things go sideways after it is removed from the DynamoDB
…delete-project
…delete-project
ethanholz
left a comment
ethanholz
left a comment
There was a problem hiding this comment.
Fix the doc comments for the stuff in this PR. Would like a followup PR updating the existing docstrings to numpy docstrings.
| """Check whether a project is currently in use. | ||
|
|
||
| Args: | ||
| project: Project name | ||
| item: Optional project item from DynamoDB | ||
|
|
||
| Returns: | ||
| Tuple of (in_use, reason) | ||
| """ |
There was a problem hiding this comment.
We aren't using numpy docstrings here. Upon further review, neither is the rest of the repo. Please update.
There was a problem hiding this comment.
Will do in follow-up PR
| ami_id: AMI ID to delete | ||
|
|
||
| Returns: | ||
| Dict with AMI cleanup details |
There was a problem hiding this comment.
Might be worth adding what this can potentially throw in the doc comments
There was a problem hiding this comment.
The PR was in progress when you added the lockfile to the repo, and I think there was a conflict early on. It has been a headache ever since; every pull is a conflict. This latest version is just from deleting the conflicting file, and regenerating it with pixi install --all
dwhswenson
left a comment
dwhswenson
left a comment
There was a problem hiding this comment.
Updates / responses made! Ready for another round, @ethanholz
| """Check whether a project is currently in use. | ||
|
|
||
| Args: | ||
| project: Project name | ||
| item: Optional project item from DynamoDB | ||
|
|
||
| Returns: | ||
| Tuple of (in_use, reason) | ||
| """ |
There was a problem hiding this comment.
Will do in follow-up PR
| ami_id: AMI ID to delete | ||
|
|
||
| Returns: | ||
| Dict with AMI cleanup details |
There was a problem hiding this comment.
The PR was in progress when you added the lockfile to the repo, and I think there was a conflict early on. It has been a headache ever since; every pull is a conflict. This latest version is just from deleting the conflicting file, and regenerating it with pixi install --all
Subcommand to delete a project. Gives two verification opportunities: before you delete from the DynamoDB table, and before you delete the actual data (AMI+snapshot). Override with
--force.This also standardizes on raising errors/returning structured data in the manager class; previously some methods returned tuples of
success_bool, message. It also adds a few helper methods to the manager class; as we move the lambda logic into the package, we might refactor them to reuse these.