Add --limit-space and --top flags for size-based cleanup prioritization#3
Add --limit-space and --top flags for size-based cleanup prioritization#3
Conversation
Co-authored-by: tejaswankalluri <49343044+tejaswankalluri@users.noreply.github.com>
Co-authored-by: tejaswankalluri <49343044+tejaswankalluri@users.noreply.github.com>
Co-authored-by: tejaswankalluri <49343044+tejaswankalluri@users.noreply.github.com>
Co-authored-by: tejaswankalluri <49343044+tejaswankalluri@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request adds two new CLI flags (--limit-space and --top) to enable size-based cleanup prioritization in docklean. Users can now clean the largest resources first or clean until a specific amount of disk space is reclaimed.
Changes:
- Added
--limit-space <size>and--top <number>CLI flags with mutual exclusivity validation - Implemented size-based filtering in the scan phase (sorting by size, selecting top N, or selecting until space limit)
- Added comprehensive test coverage with 36 new tests for parsing, validation, sorting, and selection
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types.ts | Added optional limitSpace (string) and top (number) fields to CliOptions interface |
| src/args.ts | Implemented parseSizeString() function and validation for new flags, enforced mutual exclusivity |
| src/scan.ts | Added size filtering functions (sortBySize, selectTopN, selectUntilSpaceLimit, applySizeFilters) applied after age filtering |
| src/index.ts | Integrated new limitSpaceBytes parameter from arg parsing into scan options |
| tests/args.test.ts | Added 113 new test lines covering size parsing and flag validation |
| tests/scan.test.ts | Added 337 new test lines covering size filtering, sorting, and filter composition |
| README.md | Added usage examples and flag documentation for the new features |
| package-lock.json | Version bump from 0.1.2 to 0.1.4 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/scan.test.ts
Outdated
| // Should filter for items NOT older than 14 days first (keeps recent-huge only) | ||
| // Then select top 1 from remaining (which is recent-huge) |
There was a problem hiding this comment.
The comment is confusing and doesn't clearly describe what the test is verifying. The comment says "Should filter for items NOT older than 14 days first (keeps recent-huge only)" which is a double negative and unclear. Consider rewording to: "Should filter to keep only items younger than 14 days (created within last 14 days), then select top 1 by size from the remaining items"
| // Should filter for items NOT older than 14 days first (keeps recent-huge only) | |
| // Then select top 1 from remaining (which is recent-huge) | |
| // Should filter to keep only items younger than 14 days (created within last 14 days) | |
| // Then select the single largest item by size from the remaining items (recent-huge) |
There was a problem hiding this comment.
Updated in commit ed4605f to clarify the test logic.
tests/scan.test.ts
Outdated
| // Limit to 600 MB - should select large (500) + medium (300) = 800 MB | ||
| // but since we need to reach 600, it should select large (500) first, | ||
| // then medium would exceed, so select medium anyway to reach the goal | ||
| const result = await scanResources({ | ||
| includeContainers: true, |
There was a problem hiding this comment.
The comment is confusing. Consider clarifying it to: "Limit to 600 MB - selects items in descending size order until limit is reached. Selects 'large' (500 MB, total=500 MB), then 'medium' (300 MB, total=800 MB >= limit), stops."
| // Limit to 600 MB - should select large (500) + medium (300) = 800 MB | |
| // but since we need to reach 600, it should select large (500) first, | |
| // then medium would exceed, so select medium anyway to reach the goal | |
| const result = await scanResources({ | |
| includeContainers: true, | |
| // Limit to 600 MB - selects items in descending size order until limit is reached. | |
| // Selects "large" (500 MB, total=500 MB), then "medium" (300 MB, total=800 MB >= limit), stops. | |
| const result = await scanResources({ | |
| includeContainers: true, | |
| includeContainers: true, |
| - `--limit-space <size>` Clean until specified space is reclaimed (e.g., `5GB`, `500MB`) | ||
| - `--top <number>` Select top N largest resources |
There was a problem hiding this comment.
Consider clarifying in the README that when using --top or --limit-space with multiple resource types (e.g., --containers --images --top 10), the filter applies independently to each resource type. For example, --top 10 with both containers and images would select up to 10 containers AND up to 10 images (20 items total), not 10 items across all types.
There was a problem hiding this comment.
Added clarification in commit ed4605f. The README now explains that when using --top or --limit-space with multiple resource types, the filter applies independently to each type (e.g., --top 10 with containers and images selects 10 of each, not 10 total).
| olderThanMs, | ||
| top: options.top, | ||
| limitSpaceBytes | ||
| }); |
There was a problem hiding this comment.
Critical bug: When using --top or --limit-space flags, the scan phase (lines 67-70) correctly filters resources by size, but the clean phase (around line 145, not shown in diff) uses docker prune commands that only filter by age. This means the actual cleanup will remove ALL resources matching the age filter, not just the size-filtered subset that the user confirmed. For example, if a user selects --top 5 to clean only the 5 largest images, the cleanup will actually remove all matching images, not just the 5 shown.
The implementation needs to be modified to either:
- Remove items individually by ID instead of using bulk prune commands when size filters are active, or
- Store the IDs of selected items during scan and pass them to clean for targeted removal
This is a critical issue that could cause unexpected data loss.
There was a problem hiding this comment.
@copilot apply changes based on this feedback and
also apply changes based on this feedback
There was a problem hiding this comment.
Fixed in commit ed4605f. When size filters (--top or --limit-space) are active, the cleanup now uses ID-based deletion (docker container/image/volume/network rm) instead of bulk prune commands. This ensures only the size-filtered resources shown to the user are deleted, preventing unexpected data loss.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
When --top or --limit-space flags are used, the scan phase correctly filters resources by size, but the clean phase was using bulk prune commands that would delete ALL resources matching the age filter, not just the size-filtered subset. Changes: - Added dockerRemoveContainers/Images/Volumes/Networks functions for ID-based deletion - Updated cleanResources to use ID-based deletion when selectedIds are provided - Pass selected IDs and estimated bytes from scan to clean phase - Updated README to clarify filters apply per resource type - Improved test comments for clarity Co-authored-by: tejaswankalluri <49343044+tejaswankalluri@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/args.ts
Outdated
|
|
||
| const value = Number(match[1]); | ||
| if (isNaN(value) || value < 0) { | ||
| throw new Error("Invalid size value"); |
There was a problem hiding this comment.
The error message "Invalid size value" on line 24 is generic and doesn't clearly explain the problem. It should be more specific, such as "Invalid size value: must be a positive number" to help users understand that negative values are not allowed.
| throw new Error("Invalid size value"); | |
| throw new Error("Invalid size value: must be a non-negative number"); |
There was a problem hiding this comment.
Updated in commit d3f2849 to provide a more descriptive error message.
src/clean.ts
Outdated
| output = await dockerRemoveContainers(ids); | ||
| removed[type] = ids.length; | ||
| break; | ||
| case "images": | ||
| output = await dockerRemoveImages(ids); | ||
| removed[type] = ids.length; | ||
| break; | ||
| case "volumes": | ||
| output = await dockerRemoveVolumes(ids); | ||
| removed[type] = ids.length; | ||
| break; | ||
| case "networks": | ||
| output = await dockerRemoveNetworks(ids); |
There was a problem hiding this comment.
The value assigned to output here is unused.
| output = await dockerRemoveContainers(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "images": | |
| output = await dockerRemoveImages(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "volumes": | |
| output = await dockerRemoveVolumes(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "networks": | |
| output = await dockerRemoveNetworks(ids); | |
| await dockerRemoveContainers(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "images": | |
| await dockerRemoveImages(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "volumes": | |
| await dockerRemoveVolumes(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "networks": | |
| await dockerRemoveNetworks(ids); |
src/clean.ts
Outdated
| output = await dockerRemoveContainers(ids); | ||
| removed[type] = ids.length; | ||
| break; | ||
| case "images": | ||
| output = await dockerRemoveImages(ids); | ||
| removed[type] = ids.length; | ||
| break; | ||
| case "volumes": | ||
| output = await dockerRemoveVolumes(ids); | ||
| removed[type] = ids.length; | ||
| break; | ||
| case "networks": | ||
| output = await dockerRemoveNetworks(ids); |
There was a problem hiding this comment.
The value assigned to output here is unused.
| output = await dockerRemoveContainers(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "images": | |
| output = await dockerRemoveImages(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "volumes": | |
| output = await dockerRemoveVolumes(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "networks": | |
| output = await dockerRemoveNetworks(ids); | |
| await dockerRemoveContainers(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "images": | |
| await dockerRemoveImages(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "volumes": | |
| await dockerRemoveVolumes(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "networks": | |
| await dockerRemoveNetworks(ids); |
src/clean.ts
Outdated
| removed[type] = ids.length; | ||
| break; | ||
| case "volumes": | ||
| output = await dockerRemoveVolumes(ids); |
There was a problem hiding this comment.
The value assigned to output here is unused.
There was a problem hiding this comment.
Fixed in commit d3f2849. Removed all unused output variable assignments from the ID-based deletion switch cases.
src/clean.ts
Outdated
| output = await dockerRemoveContainers(ids); | ||
| removed[type] = ids.length; | ||
| break; | ||
| case "images": | ||
| output = await dockerRemoveImages(ids); | ||
| removed[type] = ids.length; | ||
| break; | ||
| case "volumes": | ||
| output = await dockerRemoveVolumes(ids); | ||
| removed[type] = ids.length; | ||
| break; | ||
| case "networks": | ||
| output = await dockerRemoveNetworks(ids); |
There was a problem hiding this comment.
The value assigned to output here is unused.
| output = await dockerRemoveContainers(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "images": | |
| output = await dockerRemoveImages(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "volumes": | |
| output = await dockerRemoveVolumes(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "networks": | |
| output = await dockerRemoveNetworks(ids); | |
| await dockerRemoveContainers(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "images": | |
| await dockerRemoveImages(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "volumes": | |
| await dockerRemoveVolumes(ids); | |
| removed[type] = ids.length; | |
| break; | |
| case "networks": | |
| await dockerRemoveNetworks(ids); |
- Remove unused 'output' variable assignments in ID-based deletion switch cases - Improve error message for invalid size value to be more descriptive Co-authored-by: tejaswankalluri <49343044+tejaswankalluri@users.noreply.github.com>
Feature: "Top Largest" Mode Implementation - Complete ✅
All requirements from the issue have been successfully implemented and tested.
Implementation Summary:
1. New CLI Flags:
--limit-space <size>: Clean resources until specified space is reclaimed (e.g., 5GB, 500MB)--top <number>: Select top N largest resources to clean2. Core Logic:
parseDockerSize()parseSizeString()for user input parsing3. Testing:
4. Documentation:
Recent Fixes (based on code review):
Usage Examples:
Security Summary:
No security vulnerabilities detected. All changes are surgical and focused on the feature requirements.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.