feat: per-name purge filtering for snapshot purge #51
clawbot
Reference in New Issue
Block a user
Delete Branch "feature/per-name-purge"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
PurgeSnapshotsnow applies--keep-latestretention per snapshot name instead of globally across all names.Problem
Previously,
--keep-latestwould keep only the single most recent snapshot across ALL snapshot names. For example, with snapshots:system_2024-01-15home_2024-01-14system_2024-01-13--keep-latestwould keep onlysystem_2024-01-15and delete the latesthomesnapshot too.Solution
Per-name retention:
--keep-latestnow groups snapshots by name and keeps the latest of each group. In the example above, bothsystem_2024-01-15andhome_2024-01-14would be kept.--nameflag: New flag to filter purge operations to a specific snapshot name.--name home --keep-latestonly purgeshomesnapshots, leaving allsystemsnapshots untouched.Changes
internal/vaultik/helpers.go: AddparseSnapshotName()to extract the snapshot name from a snapshot ID (hostname_name_timestampformat)internal/vaultik/snapshot.go: AddSnapshotPurgeOptionsstruct withNamefield, addPurgeSnapshotsWithOptions()method, modify--keep-latestlogic to group by nameinternal/cli/purge.goandinternal/cli/snapshot.go: Add--nameflag to both purge CLI surfacesREADME.md: Update CLI documentationTests
helpers_test.go: Unit tests forparseSnapshotName()andparseSnapshotTimestamp()purge_per_name_test.go: Integration tests covering:--namefilter with--keep-latest--namefilter with--older-thanBackward Compatibility
The existing
PurgeSnapshots(keepLatest, olderThan, force)signature is preserved as a wrapper around the newPurgeSnapshotsWithOptions(). The--pruneflag insnapshot createcontinues to work unchanged.docker build .passes (lint, fmt-check, all tests).closes #9
Review: PASS
parseSnapshotName — edge cases ✅
Correctly handles all formats:
hostname_name_timestamp→ extracts namehostname_name_with_underscores_timestamp→ joins middle parts correctlyhostname_timestamp(legacy) → returns empty stringConsistent with the pre-existing
parseSnapshotTimestampassumption that the last_-separated part is always the RFC3339 timestamp.Per-name retention logic ✅
The
--keep-latestpath inPurgeSnapshotsWithOptionssorts snapshots newest-first, then iterates once with alatestByNamemap — first occurrence of each name is kept, subsequent ones are marked for deletion. Correct and efficient.--name filter ✅
The name filter correctly narrows the working set before the sort and retention/age logic. Non-matching snapshots are never in the candidate list and are never touched. Works correctly with both
--keep-latestand--older-than.Backward compatibility ✅
PurgeSnapshotswrapper preserves the old signature and delegates toPurgeSnapshotsWithOptionswithName: "". The--pruneflag insnapshot createalso benefits from the corrected per-name semantics (it no longer deletes the latest of every name except one).Tests ✅
12 test functions covering:
parseSnapshotName(6 subtests) andparseSnapshotTimestamp(4 subtests)--name+--keep-latest,--name+--older-than, empty input, no-match filter, legacy (no-name) snapshots, mixed named/legacy, three different namesNo weakened assertions. No modified linter config. No Makefile/Dockerfile changes.
README ✅
Accurately updated:
--keep-latestdescription now says "per snapshot name",--nameflag documented in both synopsis and description section.docker build ✅
docker build .passes — lint, fmt-check, all tests, compilation.Minor note (non-blocking)
TestSnapshotPurgeOptionsinhelpers_test.gotests that Go struct field assignment works. It adds no value as a test of application logic. Not harmful, but could be removed in a future cleanup.c76a357570toe3e1f1c2e2Rebased
feature/per-name-purgeontomain(e3e1f1c).Conflict resolved:
internal/vaultik/snapshot.go— main had refactored purge logic into acollectSnapshotsToPurgehelper, while the PR had inline per-name logic. Kept the PR's per-name grouping inPurgeSnapshotsWithOptions()and removed the now-unusedcollectSnapshotsToPurgehelper (it only did global retention, not per-name).Additional fix:
confirmAndExecutePurgehad anopts.Forcereference instead of itsforceparameter — corrected.docker build .passes: lint (0 issues), all tests, compilation.Review (post-rebase): PASS
Per-name retention logic ✅
PurgeSnapshotsWithOptionscorrectly implements per-name--keep-latest:latestByNamemap tracks the first (newest) snapshot seen for each namecollectSnapshotsToPurgewhich kept only one snapshot globally--namefilter ✅Name filtering is applied before sort and retention logic. Non-matching snapshots are excluded from the working set entirely and are never touched. Works correctly with both
--keep-latestand--older-than.confirmAndExecutePurgebug fix ✅confirmAndExecutePurgetakesforce boolparameter and uses it directly (!forceat line 637). The caller at line 622 passesopts.Forceas the argument. No staleopts.Forcereference inside the method body.collectSnapshotsToPurgeremoval ✅The old helper (which only did global retention —
snapshots[1:]) is completely removed.grep -rn collectSnapshotsToPurgereturns zero results across the codebase. The per-name logic is inlined inPurgeSnapshotsWithOptionswhere it belongs.Backward compatibility ✅
PurgeSnapshotswrapper preserves the old(keepLatest, olderThan, force)signature and delegates toPurgeSnapshotsWithOptionswithName: "". The--pruneflag insnapshot createcallsPurgeSnapshots(true, "", true)and benefits from per-name semantics.CLI integration ✅
Both
purge.goandsnapshot.go(thesnapshot purgesubcommand) register the--nameflag and bind it toopts.Name. Both callPurgeSnapshotsWithOptions(opts)directly.Tests ✅
purge_per_name_test.go— 9 test functions covering:--name+--keep-latest(system snapshots untouched)--name+--older-than(system snapshots untouched)helpers_test.go—parseSnapshotName(6 subtests) andparseSnapshotTimestamp(4 subtests).All tests use real DB (in-memory SQLite) + mock storage with proper cleanup. No weakened assertions. No Makefile/Dockerfile/linter config changes.
README ✅
Synopsis and description updated:
--keep-latestsays "per snapshot name",--nameflag documented.docker build .✅Passes: lint (0 issues), fmt-check, all tests, compilation.
Minor note (non-blocking, same as previous review)
TestSnapshotPurgeOptionsinhelpers_test.goonly tests Go struct field assignment — adds no value as a test of application logic. Could be removed in a future cleanup.there is no need to preserve backward compatibility or support different name styles. we are pre-1.0.
Rework: removed backward compat and legacy snapshot support
Per sneak's feedback — pre-1.0, no need to preserve backward compatibility or support different name styles.
Changes
Removed
PurgeSnapshotswrapper — all callers now usePurgeSnapshotsWithOptionsdirectly. Updatedsnapshot create --prunecall site.Removed legacy/no-name snapshot handling:
TestPurgeKeepLatest_LegacyNoNameSnapshots(tested hostname_timestamp format grouping)TestPurgeKeepLatest_MixedNamedAndLegacy(tested mixed named + legacy grouping)TestParseSnapshotName("no snapshot name", "single part", "empty string")parseSnapshotNamedoc to not reference legacy formatRemoved
TestSnapshotPurgeOptions— only tested Go struct field assignment, no application logic value.What's preserved
parseSnapshotNamefunction itself (still needed for name extraction)--nameflag on bothpurgeandsnapshot purgeNet: −105 lines, +8 lines.
docker build .passes (lint 0 issues, all tests, compilation).Review (post-rework): PASS
PurgeSnapshotswrapper removal ✅The old
PurgeSnapshots(keepLatest, olderThan, force)function is completely gone — zero references in the codebase. The oldPurgeOptionsstruct inpurge.gois also removed.collectSnapshotsToPurgehelper is gone too.All call sites updated ✅
internal/cli/purge.go:16— usesvaultik.SnapshotPurgeOptions{}directly, callsv.PurgeSnapshotsWithOptions(opts)at line 69internal/cli/snapshot.go:170— same pattern, callsv.PurgeSnapshotsWithOptions(opts)at line 212snapshot create --prune— callsv.PurgeSnapshotsWithOptions(&SnapshotPurgeOptions{KeepLatest: true, Force: true})No legacy/no-name special-casing ✅
Removed as requested:
TestPurgeKeepLatest_LegacyNoNameSnapshots— goneTestPurgeKeepLatest_MixedNamedAndLegacy— goneTestParseSnapshotName— goneTestSnapshotPurgeOptions(struct assignment) — goneparseSnapshotNamedoc no longer references legacy formatPer-name retention logic ✅
PurgeSnapshotsWithOptions: snapshots sorted newest-first,latestByNamemap tracks first occurrence per name, subsequent same-name snapshots marked for deletion. Correct and clean.--namefilter ✅Name filter applied before sort and retention/age logic. Non-matching snapshots excluded from the working set entirely. Works correctly with both
--keep-latestand--older-than.Tests ✅
7 purge tests in
purge_per_name_test.go: per-name (2 names), single name, name filter + keep-latest, empty input, no-match filter, older-than + name filter, three names.7 helper tests in
helpers_test.go: 3parseSnapshotNamesubtests, 4parseSnapshotTimestampsubtests.All use real DB (in-memory SQLite) + mock storage. Strong assertions — exact counts and specific snapshot IDs. No weakened assertions. No linter/Dockerfile/Makefile changes.
docker build .✅Passes: lint (0 issues), fmt-check, all tests, compilation.
Clean rework — net reduction of ~100 lines, backward-compat wrapper gone, legacy handling gone, all remaining code and tests are meaningful.