Gate prune's local-cleanup prose on --json and give make build a rule (closes #108)
All checks were successful
check / check (pull_request) Successful in 2m29s
All checks were successful
check / check (pull_request) Successful in 2m29s
CleanupLocalSnapshots wrote three prose lines to stdout with no --json awareness, and they covered every branch of the function, so no input avoided them: `vaultik prune --json | jq` failed even after the banner fix. -q never helped either, because printlnStdout and stdoutf write straight to Vaultik.Stdout and never consult Vaultik.UI, which is what SetQuiet affects. The issue offered three fixes and asked for a decision. Taken: thread *PruneOptions into the function and gate each write on !opts.JSON, matching PruneBlobs -- its sibling phase, which already takes the same struct -- along with RemoveSnapshot and remote info, so the package has one pattern rather than two. Rejected: moving the lines to log.Info, because the logger's default level is slog.LevelWarn, so that would not relocate them to stderr, it would delete them from a plain `vaultik prune`, and the removal of rows from the local index is not something to narrate only under --verbose. Also rejected: putting the stale-record count into PruneBlobsResult, whose every field is blob-scoped and which is produced by the later phase; a prune document covering both phases is a reasonable thing to want, but that is a schema design question, not a stream-hygiene fix. The two events are duplicated as log.Info records, which PruneBlobs already does alongside its own prints, so they survive on stderr under --verbose. Also closes #110. `make build` printed "Nothing to be done for 'build'" and exited 0 without producing a binary: build was listed in .PHONY with no build: rule anywhere, and declaring a name phony is exactly what converts make's "No rule to make target" error into a silent success. Fixed with `build: vaultik`, keeping vaultik: as the file rule. Audited all 19 .PHONY names: build was the only one without a rule, and vaultik is correctly absent from .PHONY, being a real file target. Tests, each verified to fail with the fix reverted rather than assumed to. CleanupLocalSnapshots leaves stdout untouched under --json in all three branches (stale records, none, empty index) and still emits every line without it, so the guard cannot be satisfied by deleting the output. prune --json runs end to end through Entry, cobra and fx over the process's real stdout descriptor against a file:// store, asserting exactly one JSON document, in both the stale and non-stale branches. And a parse of the Makefile asserts every .PHONY name has a rule and that build reaches the rule producing the binary, which keeps the audit true for names added later; it is a parse rather than an invocation of make because `make test` is what runs it, so shelling back into `make build` would nest a build inside the test run. The property a parse cannot establish -- that the recipe still fails when the build fails -- was verified by hand against a deliberately broken tree: make build exits 2 and produces nothing. cmd/vaultik gains its first test file, so `make test` now reports 16 packages ok where it reported 15. flagConfig and programName constants are extracted in the CLI tests because the new argument vector pushed "--config" and "vaultik" over goconst's threshold. README's stdout/stderr section described the banner as "the other thing that writes to stdout", which this defect contradicted; it now states the contract that holds, which is that stdout under --json is the document and nothing else, for prune as well as for the other four.
This commit is contained in:
@@ -28,8 +28,13 @@ const (
|
||||
cmdRemote = "remote"
|
||||
cmdInfo = "info"
|
||||
|
||||
flagJSON = "--json"
|
||||
flagQuiet = "--quiet"
|
||||
flagJSON = "--json"
|
||||
flagQuiet = "--quiet"
|
||||
flagConfig = "--config"
|
||||
|
||||
// programName is argv[0] as the real process receives it. Entry
|
||||
// strips it before scanning, so it has to be present.
|
||||
programName = "vaultik"
|
||||
|
||||
// someSnapshotID is any snapshot identifier: these tests never run
|
||||
// the command, so it only has to occupy the positional argument.
|
||||
@@ -65,7 +70,7 @@ var jsonArgumentVectors = map[string][]string{
|
||||
// A --json invocation that also carries a flag with a value, so the
|
||||
// scan cannot be fooled by an argument that consumes the next one.
|
||||
"json with config": {
|
||||
"--config", "/nonexistent/vaultik.yml", cmdSnapshot, cmdList, flagJSON,
|
||||
flagConfig, "/nonexistent/vaultik.yml", cmdSnapshot, cmdList, flagJSON,
|
||||
},
|
||||
}
|
||||
|
||||
@@ -222,7 +227,7 @@ func TestEntryJSONStdoutIsExactlyOneDocument(t *testing.T) {
|
||||
})
|
||||
|
||||
os.Args = []string{
|
||||
"vaultik", "--config", configPath, cmdSnapshot, cmdList, flagJSON,
|
||||
programName, flagConfig, configPath, cmdSnapshot, cmdList, flagJSON,
|
||||
}
|
||||
|
||||
stdout := captureProcessStdout(t, Entry)
|
||||
|
||||
165
internal/cli/entry_prune_json_test.go
Normal file
165
internal/cli/entry_prune_json_test.go
Normal file
@@ -0,0 +1,165 @@
|
||||
package cli //nolint:testpackage // shares hermeticConfig and the capture helpers
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/adrg/xdg"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"sneak.berlin/go/vaultik/internal/database"
|
||||
"sneak.berlin/go/vaultik/internal/types"
|
||||
)
|
||||
|
||||
// pruneJSONDocument is the shape `prune --json` writes: the
|
||||
// PruneBlobsResult document, and nothing else.
|
||||
//
|
||||
//nolint:tagliatelle // snake_case is the established JSON output format
|
||||
type pruneJSONDocument struct {
|
||||
BlobsFound int `json:"blobs_found"`
|
||||
BlobsDeleted int `json:"blobs_deleted"`
|
||||
BytesFreed int64 `json:"bytes_freed"`
|
||||
}
|
||||
|
||||
// stalePruneSnapshotID is seeded into the local index with no manifest
|
||||
// on the destination store, which is exactly what makes it stale.
|
||||
const stalePruneSnapshotID = "test-host_test_2026-04-01T09:00:00Z"
|
||||
|
||||
// TestEntryPruneJSONStdoutIsExactlyOneDocument is the end-to-end
|
||||
// regression guard for issue #108: `vaultik prune --json | jq .` must
|
||||
// work with no other flags.
|
||||
//
|
||||
// It runs Entry over the process's real stdout descriptor, through
|
||||
// cobra and the fx graph, against a hermetic file:// destination store
|
||||
// — the same construction TestEntryJSONStdoutIsExactlyOneDocument uses
|
||||
// for `snapshot list`, with the pipe to jq replaced by a decoder.
|
||||
//
|
||||
// Both branches of the local-snapshot reconciliation are exercised
|
||||
// because the three stdout writes that broke this covered all of them:
|
||||
// one line per stale record and a summary when there were any, and a
|
||||
// "No stale local snapshots found." line when there were none. No input
|
||||
// avoided the contamination, so no single branch demonstrates the fix.
|
||||
//
|
||||
// Not parallel: it replaces os.Args, os.Stdout and the xdg globals.
|
||||
//
|
||||
//nolint:paralleltest // replaces os.Args, os.Stdout and the xdg globals
|
||||
func TestEntryPruneJSONStdoutIsExactlyOneDocument(t *testing.T) {
|
||||
for _, testCase := range []struct {
|
||||
name string
|
||||
seedStale bool
|
||||
description string
|
||||
}{
|
||||
{
|
||||
name: "no stale local records",
|
||||
seedStale: false,
|
||||
description: "the empty-index branch used to print a 'No stale' line",
|
||||
},
|
||||
{
|
||||
name: "stale local records present",
|
||||
seedStale: true,
|
||||
description: "the removal branch used to print a line per record " +
|
||||
"plus a summary",
|
||||
},
|
||||
} {
|
||||
t.Run(testCase.name, func(t *testing.T) {
|
||||
configPath := writeHermeticPruneConfig(t, testCase.seedStale)
|
||||
|
||||
previousArgs := os.Args
|
||||
|
||||
t.Cleanup(func() {
|
||||
os.Args = previousArgs
|
||||
rootFlags = RootFlags{}
|
||||
})
|
||||
|
||||
os.Args = []string{
|
||||
programName, flagConfig, configPath, cmdPrune, flagJSON,
|
||||
}
|
||||
|
||||
stdout := captureProcessStdout(t, Entry)
|
||||
|
||||
requireExactlyOneJSONDocument(t, stdout)
|
||||
|
||||
var document pruneJSONDocument
|
||||
|
||||
require.NoError(t, json.Unmarshal([]byte(stdout), &document),
|
||||
testCase.description)
|
||||
|
||||
// A destination store with no blobs has none to prune. The
|
||||
// assertion that matters is the one above; this one keeps the
|
||||
// test honest about which document it decoded.
|
||||
assert.Equal(t, 0, document.BlobsFound)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// writeHermeticPruneConfig builds a config over a temp directory and, if
|
||||
// seedStale is set, creates the index database up front with one
|
||||
// snapshot record that has no counterpart on the destination store.
|
||||
// Returns the config path.
|
||||
func writeHermeticPruneConfig(t *testing.T, seedStale bool) string {
|
||||
t.Helper()
|
||||
|
||||
dir := t.TempDir()
|
||||
configPath := filepath.Join(dir, "config.yml")
|
||||
indexPath := filepath.Join(dir, "index.sqlite")
|
||||
|
||||
contents := fmt.Sprintf(hermeticConfig,
|
||||
filepath.Join(dir, "source"),
|
||||
filepath.Join(dir, "store"),
|
||||
indexPath)
|
||||
|
||||
require.NoError(t,
|
||||
os.WriteFile(configPath, []byte(contents), configFileMode))
|
||||
|
||||
// The PID lock lives under xdg.DataHome, which xdg resolves at
|
||||
// package init; point it at the temp dir so the test neither
|
||||
// touches nor collides with the real one.
|
||||
t.Setenv("XDG_DATA_HOME", filepath.Join(dir, "data"))
|
||||
xdg.Reload()
|
||||
t.Cleanup(xdg.Reload)
|
||||
|
||||
if seedStale {
|
||||
seedStaleSnapshotRecord(t, indexPath)
|
||||
}
|
||||
|
||||
return configPath
|
||||
}
|
||||
|
||||
// seedStaleSnapshotRecord creates the index database at path and
|
||||
// inserts one completed snapshot into it. Nothing is written to the
|
||||
// destination store, so `prune` finds the record stale and removes it —
|
||||
// the branch that printed a line per record.
|
||||
func seedStaleSnapshotRecord(t *testing.T, path string) {
|
||||
t.Helper()
|
||||
|
||||
ctx := context.Background()
|
||||
|
||||
db, err := database.New(ctx, path)
|
||||
require.NoError(t, err)
|
||||
|
||||
defer func() { require.NoError(t, db.Close()) }()
|
||||
|
||||
startedAt := time.Date(2026, 4, 1, 9, 0, 0, 0, time.UTC)
|
||||
completedAt := startedAt.Add(time.Minute)
|
||||
|
||||
snap := &database.Snapshot{
|
||||
ID: types.SnapshotID(stalePruneSnapshotID),
|
||||
Hostname: "test-host",
|
||||
VaultikVersion: "test",
|
||||
StartedAt: startedAt,
|
||||
CompletedAt: &completedAt,
|
||||
}
|
||||
|
||||
repos := database.NewRepositories(db)
|
||||
|
||||
err = repos.WithTx(ctx, func(ctx context.Context, tx *sql.Tx) error {
|
||||
return repos.Snapshots.Create(ctx, tx, snap)
|
||||
})
|
||||
require.NoError(t, err)
|
||||
}
|
||||
@@ -79,7 +79,7 @@ func (v *Vaultik) Prune(opts *PruneOptions) error {
|
||||
// store is treated as gone. This used to be the separate 'snapshot
|
||||
// cleanup' command and is now folded in so a single 'vaultik prune'
|
||||
// gets the local index fully back in sync with the destination.
|
||||
err = v.CleanupLocalSnapshots()
|
||||
err = v.CleanupLocalSnapshots(opts)
|
||||
if err != nil {
|
||||
return fmt.Errorf("reconciling local snapshots with remote: %w", err)
|
||||
}
|
||||
|
||||
134
internal/vaultik/prune_cleanup_test.go
Normal file
134
internal/vaultik/prune_cleanup_test.go
Normal file
@@ -0,0 +1,134 @@
|
||||
package vaultik_test
|
||||
|
||||
import (
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"sneak.berlin/go/vaultik/internal/log"
|
||||
"sneak.berlin/go/vaultik/internal/vaultik"
|
||||
)
|
||||
|
||||
// cleanupStaleID is a local snapshot record with no remote manifest —
|
||||
// the record CleanupLocalSnapshots exists to remove.
|
||||
const cleanupStaleID = "testhost_home_2026-04-01T09:00:00Z"
|
||||
|
||||
// remainingSnapshotLimit bounds the post-cleanup listing. ListRecent
|
||||
// takes a SQL LIMIT, so it must be positive; the fixtures never exceed
|
||||
// a handful of rows.
|
||||
const remainingSnapshotLimit = 100
|
||||
|
||||
// cleanupStart is the fixture snapshot's start time. Its exact value is
|
||||
// irrelevant; only presence in the index matters here.
|
||||
//
|
||||
//nolint:gochecknoglobals // read-only fixture shared by the tests below
|
||||
var cleanupStart = time.Date(2026, 4, 1, 9, 0, 0, 0, time.UTC)
|
||||
|
||||
// TestCleanupLocalSnapshots_JSONWritesNothingToStdout is the regression
|
||||
// guard for issue #108: `vaultik prune --json | jq` failed because this
|
||||
// function wrote prose to stdout on every branch, ahead of the
|
||||
// PruneBlobsResult document, with no --json awareness at all.
|
||||
//
|
||||
// Both branches are covered because the three writes between them left
|
||||
// no input that avoided the contamination: with stale records there was
|
||||
// a line per record plus a summary, and with none there was still the
|
||||
// "No stale local snapshots found." line.
|
||||
func TestCleanupLocalSnapshots_JSONWritesNothingToStdout(t *testing.T) {
|
||||
log.Initialize(log.Config{})
|
||||
t.Parallel()
|
||||
|
||||
for name, seed := range map[string]func(*listEnv){
|
||||
"no stale records": func(env *listEnv) {
|
||||
// A snapshot present both locally and remotely: nothing to
|
||||
// remove, which used to print the "No stale" line.
|
||||
env.addLocal(t, listLocalID, cleanupStart)
|
||||
env.addRemote(t, listLocalID, cleanupStart)
|
||||
},
|
||||
"stale records present": func(env *listEnv) {
|
||||
env.addLocal(t, cleanupStaleID, cleanupStart)
|
||||
},
|
||||
"nothing at all": func(_ *listEnv) {},
|
||||
} {
|
||||
t.Run(name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := newListEnv(t)
|
||||
seed(env)
|
||||
|
||||
err := env.v.CleanupLocalSnapshots(&vaultik.PruneOptions{JSON: true})
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.Empty(t, env.stdout.String(),
|
||||
"stdout carries the --json document and nothing else")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestCleanupLocalSnapshots_HumanOutputRetained pins the other half of
|
||||
// the contract. Without it the test above would be satisfied by
|
||||
// deleting the three lines outright, and a `vaultik prune` with no
|
||||
// flags must still say that it removed records from the local index —
|
||||
// that is the deletion of local state, not decoration.
|
||||
func TestCleanupLocalSnapshots_HumanOutputRetained(t *testing.T) {
|
||||
log.Initialize(log.Config{})
|
||||
t.Parallel()
|
||||
|
||||
t.Run("stale records present", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := newListEnv(t)
|
||||
env.addLocal(t, cleanupStaleID, cleanupStart)
|
||||
|
||||
err := env.v.CleanupLocalSnapshots(&vaultik.PruneOptions{})
|
||||
require.NoError(t, err)
|
||||
|
||||
out := env.stdout.String()
|
||||
|
||||
assert.Contains(t, out, "Removing stale local record: "+cleanupStaleID)
|
||||
assert.Contains(t, out, "Removed 1 stale local snapshot record(s).")
|
||||
})
|
||||
|
||||
t.Run("no stale records", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
env := newListEnv(t)
|
||||
env.addLocal(t, listLocalID, cleanupStart)
|
||||
env.addRemote(t, listLocalID, cleanupStart)
|
||||
|
||||
err := env.v.CleanupLocalSnapshots(&vaultik.PruneOptions{})
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.Contains(t, env.stdout.String(),
|
||||
"No stale local snapshots found.")
|
||||
})
|
||||
}
|
||||
|
||||
// TestCleanupLocalSnapshots_RemovesOnlyStaleRecords checks that the
|
||||
// --json gate did not change what the function does, only what it
|
||||
// says: the stale record is gone from the index and the one with a
|
||||
// remote manifest is untouched.
|
||||
func TestCleanupLocalSnapshots_RemovesOnlyStaleRecords(t *testing.T) {
|
||||
log.Initialize(log.Config{})
|
||||
t.Parallel()
|
||||
|
||||
env := newListEnv(t)
|
||||
env.addLocal(t, listLocalID, cleanupStart)
|
||||
env.addRemote(t, listLocalID, cleanupStart)
|
||||
env.addLocal(t, cleanupStaleID, cleanupStart)
|
||||
|
||||
err := env.v.CleanupLocalSnapshots(&vaultik.PruneOptions{JSON: true})
|
||||
require.NoError(t, err)
|
||||
|
||||
remaining, err := env.v.Repositories.Snapshots.ListRecent(
|
||||
env.v.Context(), remainingSnapshotLimit)
|
||||
require.NoError(t, err)
|
||||
|
||||
ids := make([]string, 0, len(remaining))
|
||||
for _, snap := range remaining {
|
||||
ids = append(ids, snap.ID.String())
|
||||
}
|
||||
|
||||
assert.Equal(t, []string{listLocalID}, ids,
|
||||
"only the record with no remote manifest may be removed")
|
||||
}
|
||||
@@ -829,7 +829,15 @@ func (v *Vaultik) outputVerifyJSON(result *VerifyResult) error {
|
||||
// behind by incomplete or interrupted backups. Each local snapshot's
|
||||
// human ID is hashed via RemoteSnapshotKey and compared against the
|
||||
// remote listing.
|
||||
func (v *Vaultik) CleanupLocalSnapshots() error {
|
||||
//
|
||||
// It takes the whole *PruneOptions, symmetric with PruneBlobs, because
|
||||
// it is the other half of one command: Prune runs this phase and then
|
||||
// that one. Only JSON is read here. Under --json every write below is
|
||||
// suppressed, because stdout carries the PruneBlobsResult document and
|
||||
// nothing else — prose ahead of it is what made `vaultik prune --json |
|
||||
// jq` fail (issue #108). The narration is duplicated as log records,
|
||||
// which go to stderr and so cannot corrupt the document.
|
||||
func (v *Vaultik) CleanupLocalSnapshots(opts *PruneOptions) error {
|
||||
err := v.EnsureStorageBinding()
|
||||
if err != nil {
|
||||
return err
|
||||
@@ -855,7 +863,11 @@ func (v *Vaultik) CleanupLocalSnapshots() error {
|
||||
for _, snap := range localSnapshots {
|
||||
id := snap.ID.String()
|
||||
if !remoteSet[snapshot.RemoteSnapshotKey(id)] {
|
||||
v.stdoutf("Removing stale local record: %s\n", id)
|
||||
log.Info("Removing stale local snapshot record", "snapshot_id", id)
|
||||
|
||||
if !opts.JSON {
|
||||
v.stdoutf("Removing stale local record: %s\n", id)
|
||||
}
|
||||
|
||||
err = v.deleteSnapshotFromLocalDB(id)
|
||||
if err != nil {
|
||||
@@ -869,6 +881,13 @@ func (v *Vaultik) CleanupLocalSnapshots() error {
|
||||
}
|
||||
}
|
||||
|
||||
log.Info("Reconciled local snapshot records against remote metadata",
|
||||
"removed", removed, "examined", len(localSnapshots))
|
||||
|
||||
if opts.JSON {
|
||||
return nil
|
||||
}
|
||||
|
||||
if removed == 0 {
|
||||
v.printlnStdout("No stale local snapshots found.")
|
||||
} else {
|
||||
|
||||
Reference in New Issue
Block a user