feat: add dr docker entity for container/image/volume removal and prune#39
feat: add dr docker entity for container/image/volume removal and prune#39
Conversation
|
@greptile |
|
/gemini review |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds new functionality to the dr command for managing Docker resources, including removing containers, images, and volumes, as well as a system prune option. The changes are well-structured and include new tests. I have a couple of suggestions to improve the user experience of the confirmation prompt and to enhance test coverage for the new Docker commands.
utility.sh
Outdated
| else | ||
| echo "This will remove all stopped containers, unused images, and build cache. Continue? (y/n)" | ||
| read -r confirm | ||
| if [[ "${confirm,,}" == "y" ]]; then |
There was a problem hiding this comment.
This confirmation check only accepts 'y' or 'Y'. It would be more user-friendly to also accept 'yes'. You can use a regular expression for a more robust check.
| if [[ "${confirm,,}" == "y" ]]; then | |
| if [[ "${confirm,,}" =~ ^(y|yes)$ ]]; then |
| # Test: dr docker without subcommand shows usage | ||
| result=$(u7 dr docker 2>&1) | ||
| assert_contains "dr docker shows usage" "Usage:" "$result" | ||
|
|
||
| # Test: dr docker prune dry-run | ||
| result=$(u7 --dry-run dr docker prune 2>&1) | ||
| assert_contains "dr docker prune dry-run works" "[dry-run]" "$result" |
There was a problem hiding this comment.
The added tests for usage and dry-run are a good start. However, they don't cover the core functionality of removing Docker entities (containers, images, volumes) or the interactive prune. Consider adding more comprehensive tests for these cases. These tests could be conditionally run if Docker is available, similar to the existing sh docker containers test.
Greptile SummaryThis PR adds a new All concerns raised in prior review threads have been addressed in this revision:
Two minor P2 items remain:
Confidence Score: 5/5Safe to merge — all previously raised P0/P1 concerns are resolved; only minor P2 style suggestions remain. All critical issues from previous review threads (dry-run/install-check ordering, stdin redirect, abort exit code, _u7_exec consistency) are correctly fixed. Remaining findings are P2 style/documentation items that do not block correct operation. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["u7 dr docker <subcommand> [id]"] --> B{subcommand?}
B -->|empty| C["print Usage → return 0"]
B -->|unknown| D["print Usage → return 1"]
B -->|container / image / volume| E{dry-run?}
E -->|yes| F["skip docker check"]
E -->|no| G{docker installed?}
G -->|no| H["Error: docker not in PATH → return 1"]
G -->|yes| F
F --> I{id provided?}
I -->|no| J["print subcommand Usage → return 1"]
I -->|yes| K["_u7_exec docker rm -f / rmi / volume rm"]
B -->|prune| L{dry-run?}
L -->|yes| M["echo [dry-run] docker system prune -af → return 0"]
L -->|no| N{docker installed?}
N -->|no| H
N -->|yes| O["Prompt: Continue? (y/n) from /dev/tty"]
O -->|y| P["_u7_exec docker system prune -af"]
O -->|other| Q["echo Aborted → return 1"]
Prompt To Fix All With AIThis is a comment left during a code review.
Path: lib/drop.sh
Line: 190
Comment:
**Prune confirmation message omits unused networks**
`docker system prune -af` also removes all unused networks, but the confirmation text only mentions stopped containers, unused images, and build cache. Users could be surprised to find their custom Docker networks removed.
```suggestion
echo "This will remove all stopped containers, unused networks, unused images, and build cache. Continue? (y/n)"
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: lib/drop.sh
Line: 173-180
Comment:
**Inconsistent force-removal flags between `container` and `image`**
`docker rm -f` force-removes a running container, but `docker rmi` (no `-f`) will fail if the image has multiple tags or is referenced by a stopped container. Users who run `u7 dr docker image <id>` on a multi-tagged image will get a confusing Docker error instead of a clear message. Either document this behaviour, or use the modern canonical subcommand forms consistently:
```bash
# container
_u7_exec docker container rm -f "$2"
# image — deliberately no -f to prevent accidental removal of shared images
_u7_exec docker image rm "$2"
# volume
_u7_exec docker volume rm "$2"
```
At minimum, adding a comment to explain that `-f` is intentionally omitted for `image` (to avoid removing images still referenced by other tags) would prevent future confusion.
How can I resolve this? If you propose a fix, please make it concise.Reviews (8): Last reviewed commit: "feat: add dr docker entity with dry-run ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
@greptile |
|
@greptile |
2 similar comments
|
@greptile |
|
@greptile |
|
@greptile |
1 similar comment
|
@greptile |
vitali87
force-pushed
the
feat/dr-docker
branch
from
cffc12a to
250b6ec
Compare
|
@greptile |
1 participant
Summary
dr docker container <id>— force remove a containerdr docker image <id>— remove an imagedr docker volume <name>— remove a volumedr docker prune— system prune with y/n confirmation (dry-run supported)Test plan
u7 dr dockershows usageu7 --dry-run dr docker pruneshows dry-run output