sneak/vaultik
1
0
Fork 0
Code Issues 7 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

feat: per-name purge filtering for snapshot purge #51

Open
clawbot wants to merge 1 commits from feature/per-name-purge into main
pull from: feature/per-name-purge
merge into: sneak:main
Conversation 3 Commits 1 Files Changed 7 +532 -59
clawbot commented 2026-03-18 05:54:27 +01:00
Collaborator
Copy Link

Summary

PurgeSnapshots now applies --keep-latest retention per snapshot name instead of globally across all names.

Problem

Previously, --keep-latest would keep only the single most recent snapshot across ALL snapshot names. For example, with snapshots:

  • system_2024-01-15
  • home_2024-01-14
  • system_2024-01-13

--keep-latest would keep only system_2024-01-15 and delete the latest home snapshot too.

Solution

  1. Per-name retention: --keep-latest now groups snapshots by name and keeps the latest of each group. In the example above, both system_2024-01-15 and home_2024-01-14 would be kept.

  2. --name flag: New flag to filter purge operations to a specific snapshot name. --name home --keep-latest only purges home snapshots, leaving all system snapshots untouched.

Changes

  • internal/vaultik/helpers.go: Add parseSnapshotName() to extract the snapshot name from a snapshot ID (hostname_name_timestamp format)
  • internal/vaultik/snapshot.go: Add SnapshotPurgeOptions struct with Name field, add PurgeSnapshotsWithOptions() method, modify --keep-latest logic to group by name
  • internal/cli/purge.go and internal/cli/snapshot.go: Add --name flag to both purge CLI surfaces
  • README.md: Update CLI documentation

Tests

  • helpers_test.go: Unit tests for parseSnapshotName() and parseSnapshotTimestamp()
  • purge_per_name_test.go: Integration tests covering:
    • Per-name retention with multiple names
    • Single-name retention
    • --name filter with --keep-latest
    • --name filter with --older-than
    • No-match name filter (all snapshots retained)
    • Legacy snapshots without name component
    • Mixed named and legacy snapshots
    • Three different snapshot names

Backward Compatibility

The existing PurgeSnapshots(keepLatest, olderThan, force) signature is preserved as a wrapper around the new PurgeSnapshotsWithOptions(). The --prune flag in snapshot create continues to work unchanged.

docker build . passes (lint, fmt-check, all tests).

closes #9

## Summary `PurgeSnapshots` now applies `--keep-latest` retention per snapshot name instead of globally across all names. ### Problem Previously, `--keep-latest` would keep only the single most recent snapshot across ALL snapshot names. For example, with snapshots: - `system_2024-01-15` - `home_2024-01-14` - `system_2024-01-13` `--keep-latest` would keep only `system_2024-01-15` and delete the latest `home` snapshot too. ### Solution 1. **Per-name retention**: `--keep-latest` now groups snapshots by name and keeps the latest of each group. In the example above, both `system_2024-01-15` and `home_2024-01-14` would be kept. 2. **`--name` flag**: New flag to filter purge operations to a specific snapshot name. `--name home --keep-latest` only purges `home` snapshots, leaving all `system` snapshots untouched. ### Changes - `internal/vaultik/helpers.go`: Add `parseSnapshotName()` to extract the snapshot name from a snapshot ID (`hostname_name_timestamp` format) - `internal/vaultik/snapshot.go`: Add `SnapshotPurgeOptions` struct with `Name` field, add `PurgeSnapshotsWithOptions()` method, modify `--keep-latest` logic to group by name - `internal/cli/purge.go` and `internal/cli/snapshot.go`: Add `--name` flag to both purge CLI surfaces - `README.md`: Update CLI documentation ### Tests - `helpers_test.go`: Unit tests for `parseSnapshotName()` and `parseSnapshotTimestamp()` - `purge_per_name_test.go`: Integration tests covering: - Per-name retention with multiple names - Single-name retention - `--name` filter with `--keep-latest` - `--name` filter with `--older-than` - No-match name filter (all snapshots retained) - Legacy snapshots without name component - Mixed named and legacy snapshots - Three different snapshot names ### Backward Compatibility The existing `PurgeSnapshots(keepLatest, olderThan, force)` signature is preserved as a wrapper around the new `PurgeSnapshotsWithOptions()`. The `--prune` flag in `snapshot create` continues to work unchanged. `docker build .` passes (lint, fmt-check, all tests). closes [#9](https://git.eeqj.de/sneak/vaultik/issues/9)
clawbot added 1 commit 2026-03-18 05:54:27 +01:00
feat: per-name purge filtering for snapshot purge
All checks were successful
check / check (pull_request) Successful in 4m13s
Details
c76a357570 
PurgeSnapshots now applies --keep-latest retention per snapshot name
instead of globally across all names. Previously, --keep-latest would
keep only the single most recent snapshot regardless of name, deleting
the latest snapshots of other names (e.g. keeping only the newest
'system' snapshot while deleting all 'home' snapshots).

Changes:
- Add parseSnapshotName() to extract snapshot name from snapshot IDs
- Add SnapshotPurgeOptions struct with Name field for --name filtering
- Add PurgeSnapshotsWithOptions() method accepting full options
- Modify --keep-latest to group snapshots by name and keep the latest
  per group (backward compatible: PurgeSnapshots() wrapper preserved)
- Add --name flag to both 'vaultik purge' and 'vaultik snapshot purge'
  CLI commands to filter purge operations to a specific snapshot name
- Add comprehensive tests for per-name purge behavior including:
  multi-name retention, name filtering, legacy/mixed format support,
  older-than with name filter, and edge cases

closes #9
clawbot added the needs-review label 2026-03-18 05:55:06 +01:00
clawbot reviewed 2026-03-18 05:59:30 +01:00
clawbot left a comment
Author
Collaborator
Copy Link

Review: PASS

parseSnapshotName — edge cases ✅

Correctly handles all formats:

  • hostname_name_timestamp → extracts name
  • hostname_name_with_underscores_timestamp → joins middle parts correctly
  • hostname_timestamp (legacy) → returns empty string
  • Empty string / single part → returns empty string

Consistent with the pre-existing parseSnapshotTimestamp assumption that the last _-separated part is always the RFC3339 timestamp.

Per-name retention logic ✅

The --keep-latest path in PurgeSnapshotsWithOptions sorts snapshots newest-first, then iterates once with a latestByName map — 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-latest and --older-than.

Backward compatibility ✅

PurgeSnapshots wrapper preserves the old signature and delegates to PurgeSnapshotsWithOptions with Name: "". The --prune flag in snapshot create also benefits from the corrected per-name semantics (it no longer deletes the latest of every name except one).

Tests ✅

12 test functions covering:

  • Unit tests for parseSnapshotName (6 subtests) and parseSnapshotTimestamp (4 subtests)
  • Integration tests for purge: per-name retention with multiple names, single name, --name + --keep-latest, --name + --older-than, empty input, no-match filter, legacy (no-name) snapshots, mixed named/legacy, three different names
  • All use real DB (in-memory SQLite) + mock storage with proper cleanup

No weakened assertions. No modified linter config. No Makefile/Dockerfile changes.

README ✅

Accurately updated: --keep-latest description now says "per snapshot name", --name flag documented in both synopsis and description section.

docker build ✅

docker build . passes — lint, fmt-check, all tests, compilation.

Minor note (non-blocking)

TestSnapshotPurgeOptions in helpers_test.go tests 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.

## Review: PASS ### parseSnapshotName — edge cases ✅ Correctly handles all formats: - `hostname_name_timestamp` → extracts name - `hostname_name_with_underscores_timestamp` → joins middle parts correctly - `hostname_timestamp` (legacy) → returns empty string - Empty string / single part → returns empty string Consistent with the pre-existing `parseSnapshotTimestamp` assumption that the last `_`-separated part is always the RFC3339 timestamp. ### Per-name retention logic ✅ The `--keep-latest` path in [`PurgeSnapshotsWithOptions`](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/vaultik/snapshot.go#L558-L570) sorts snapshots newest-first, then iterates once with a `latestByName` map — first occurrence of each name is kept, subsequent ones are marked for deletion. Correct and efficient. ### --name filter ✅ The [name filter](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/vaultik/snapshot.go#L538-L546) 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-latest` and `--older-than`. ### Backward compatibility ✅ [`PurgeSnapshots`](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/vaultik/snapshot.go#L497-L503) wrapper preserves the old signature and delegates to `PurgeSnapshotsWithOptions` with `Name: ""`. The `--prune` flag in `snapshot create` also benefits from the corrected per-name semantics (it no longer deletes the latest of every name except one). ### Tests ✅ 12 test functions covering: - Unit tests for `parseSnapshotName` (6 subtests) and `parseSnapshotTimestamp` (4 subtests) - Integration tests for purge: per-name retention with multiple names, single name, `--name` + `--keep-latest`, `--name` + `--older-than`, empty input, no-match filter, legacy (no-name) snapshots, mixed named/legacy, three different names - All use real DB (in-memory SQLite) + mock storage with proper cleanup No weakened assertions. No modified linter config. No Makefile/Dockerfile changes. ### README ✅ Accurately updated: `--keep-latest` description now says "per snapshot name", `--name` flag documented in both synopsis and description section. ### docker build ✅ `docker build .` passes — lint, fmt-check, all tests, compilation. ### Minor note (non-blocking) `TestSnapshotPurgeOptions` in [`helpers_test.go`](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/vaultik/helpers_test.go#L92-L119) tests 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.
clawbot added merge-ready and removed needs-review labels 2026-03-18 05:59:49 +01:00
sneak was assigned by clawbot 2026-03-18 05:59:50 +01:00
clawbot force-pushed feature/per-name-purge from c76a357570 to e3e1f1c2e2 2026-03-20 06:56:09 +01:00 Compare
clawbot commented 2026-03-20 06:56:18 +01:00
Author
Collaborator
Copy Link

Rebased feature/per-name-purge onto main (e3e1f1c).

Conflict resolved: internal/vaultik/snapshot.go — main had refactored purge logic into a collectSnapshotsToPurge helper, while the PR had inline per-name logic. Kept the PR's per-name grouping in PurgeSnapshotsWithOptions() and removed the now-unused collectSnapshotsToPurge helper (it only did global retention, not per-name).

Additional fix: confirmAndExecutePurge had an opts.Force reference instead of its force parameter — corrected.

docker build . passes: lint (0 issues), all tests, compilation.

Rebased `feature/per-name-purge` onto `main` (`e3e1f1c`). **Conflict resolved:** `internal/vaultik/snapshot.go` — main had refactored purge logic into a `collectSnapshotsToPurge` helper, while the PR had inline per-name logic. Kept the PR's per-name grouping in `PurgeSnapshotsWithOptions()` and removed the now-unused `collectSnapshotsToPurge` helper (it only did global retention, not per-name). **Additional fix:** `confirmAndExecutePurge` had an `opts.Force` reference instead of its `force` parameter — corrected. `docker build .` passes: lint (0 issues), all tests, compilation.
clawbot added needs-review and removed merge-ready labels 2026-03-20 06:59:49 +01:00
clawbot reviewed 2026-03-20 07:01:57 +01:00
clawbot left a comment
Author
Collaborator
Copy Link

Review (post-rebase): PASS

Per-name retention logic ✅

PurgeSnapshotsWithOptions correctly implements per-name --keep-latest:

  • Snapshots are sorted newest-first
  • A latestByName map tracks the first (newest) snapshot seen for each name
  • Subsequent snapshots with the same name are marked for deletion
  • This replaces the old collectSnapshotsToPurge which kept only one snapshot globally

--name filter ✅

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-latest and --older-than.

confirmAndExecutePurge bug fix ✅

confirmAndExecutePurge takes force bool parameter and uses it directly (!force at line 637). The caller at line 622 passes opts.Force as the argument. No stale opts.Force reference inside the method body.

collectSnapshotsToPurge removal ✅

The old helper (which only did global retention — snapshots[1:]) is completely removed. grep -rn collectSnapshotsToPurge returns zero results across the codebase. The per-name logic is inlined in PurgeSnapshotsWithOptions where it belongs.

Backward compatibility ✅

PurgeSnapshots wrapper preserves the old (keepLatest, olderThan, force) signature and delegates to PurgeSnapshotsWithOptions with Name: "". The --prune flag in snapshot create calls PurgeSnapshots(true, "", true) and benefits from per-name semantics.

CLI integration ✅

Both purge.go and snapshot.go (the snapshot purge subcommand) register the --name flag and bind it to opts.Name. Both call PurgeSnapshotsWithOptions(opts) directly.

Tests ✅

purge_per_name_test.go — 9 test functions covering:

  • Per-name retention with 2 names, single name, 3 names
  • --name + --keep-latest (system snapshots untouched)
  • --name + --older-than (system snapshots untouched)
  • No-match name filter (all retained)
  • Empty input
  • Legacy (no-name) snapshots grouped under empty string
  • Mixed named + legacy snapshots

helpers_test.go — parseSnapshotName (6 subtests) and parseSnapshotTimestamp (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-latest says "per snapshot name", --name flag documented.

docker build . ✅

Passes: lint (0 issues), fmt-check, all tests, compilation.

Minor note (non-blocking, same as previous review)

TestSnapshotPurgeOptions in helpers_test.go only tests Go struct field assignment — adds no value as a test of application logic. Could be removed in a future cleanup.

## Review (post-rebase): PASS ### Per-name retention logic ✅ [`PurgeSnapshotsWithOptions`](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/vaultik/snapshot.go#L558-L621) correctly implements per-name `--keep-latest`: - Snapshots are sorted newest-first - A `latestByName` map tracks the first (newest) snapshot seen for each name - Subsequent snapshots with the same name are marked for deletion - This replaces the old `collectSnapshotsToPurge` which kept only one snapshot globally ### `--name` filter ✅ [Name filtering](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/vaultik/snapshot.go#L569-L577) 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-latest` and `--older-than`. ### `confirmAndExecutePurge` bug fix ✅ [`confirmAndExecutePurge`](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/vaultik/snapshot.go#L626) takes `force bool` parameter and uses it directly (`!force` at line 637). The caller at line 622 passes `opts.Force` as the argument. No stale `opts.Force` reference inside the method body. ### `collectSnapshotsToPurge` removal ✅ The old helper (which only did global retention — `snapshots[1:]`) is completely removed. `grep -rn collectSnapshotsToPurge` returns zero results across the codebase. The per-name logic is inlined in `PurgeSnapshotsWithOptions` where it belongs. ### Backward compatibility ✅ [`PurgeSnapshots`](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/vaultik/snapshot.go#L540-L546) wrapper preserves the old `(keepLatest, olderThan, force)` signature and delegates to `PurgeSnapshotsWithOptions` with `Name: ""`. The `--prune` flag in `snapshot create` calls `PurgeSnapshots(true, "", true)` and benefits from per-name semantics. ### CLI integration ✅ Both [`purge.go`](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/cli/purge.go) and [`snapshot.go`](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/cli/snapshot.go) (the `snapshot purge` subcommand) register the `--name` flag and bind it to `opts.Name`. Both call `PurgeSnapshotsWithOptions(opts)` directly. ### Tests ✅ [`purge_per_name_test.go`](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/vaultik/purge_per_name_test.go) — 9 test functions covering: - Per-name retention with 2 names, single name, 3 names - `--name` + `--keep-latest` (system snapshots untouched) - `--name` + `--older-than` (system snapshots untouched) - No-match name filter (all retained) - Empty input - Legacy (no-name) snapshots grouped under empty string - Mixed named + legacy snapshots [`helpers_test.go`](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/vaultik/helpers_test.go) — `parseSnapshotName` (6 subtests) and `parseSnapshotTimestamp` (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-latest` says "per snapshot name", `--name` flag documented. ### `docker build .` ✅ Passes: lint (0 issues), fmt-check, all tests, compilation. ### Minor note (non-blocking, same as previous review) `TestSnapshotPurgeOptions` in [`helpers_test.go`](https://git.eeqj.de/sneak/vaultik/src/branch/feature/per-name-purge/internal/vaultik/helpers_test.go#L92-L119) only tests Go struct field assignment — adds no value as a test of application logic. Could be removed in a future cleanup.
clawbot added merge-ready and removed needs-review labels 2026-03-20 07:02:28 +01:00
All checks were successful
check / check (pull_request) Successful in 4m28s
Details
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feature/per-name-purge:feature/per-name-purge
git checkout feature/per-name-purge
Sign in to join this conversation.
Reviewers
No Reviewers
clawbot
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-review
needs-review
needs-review
needs-review
needs-rework
Code review found issues to fix
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/vaultik#51
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?