From 1a584cb16e4df2782996bbfa5b9f4493485ffb46 Mon Sep 17 00:00:00 2001 From: Torben Giesselmann Date: Tue, 14 Mar 2023 19:29:08 -0700 Subject: [PATCH] Refactor policy sum calculation & duration parsing - Convert policy sum calculation to function and move to tests. - Remove parseDuration(...) and use ParseDurationOrPanic(...) instead. --- internal/restic/snapshot_policy.go | 16 ++----- internal/restic/snapshot_policy_test.go | 61 ++++++++++++------------- 2 files changed, 35 insertions(+), 42 deletions(-) diff --git a/internal/restic/snapshot_policy.go b/internal/restic/snapshot_policy.go index d63cf45e3..a9ec4a9f7 100644 --- a/internal/restic/snapshot_policy.go +++ b/internal/restic/snapshot_policy.go @@ -100,13 +100,7 @@ func (e ExpirePolicy) String() (s string) { return s } -// Sum returns the maximum number of snapshots to be kept according to this -// policy. -func (e ExpirePolicy) Sum() int { - return e.Last + e.Hourly + e.Daily + e.Weekly + e.Monthly + e.Yearly -} - -// Empty returns true iff no policy has been configured (all values zero). +// Empty returns true if no policy has been configured (all values zero). func (e ExpirePolicy) Empty() bool { if len(e.Tags) != 0 { return false @@ -260,15 +254,15 @@ func ApplyPolicy(list Snapshots, p ExpirePolicy) (keep, remove Snapshots, reason // Now update the other buckets and see if they have some counts left. for i, b := range buckets { - if b.Count <= -1 || b.Count > 0 { + // -1 means "keep all" + if b.Count > 0 || b.Count == -1 { val := b.bucker(cur.Time, nr) if val != b.Last { debug.Log("keep %v %v, bucker %v, val %v\n", cur.Time, cur.id.Str(), i, val) keepSnap = true buckets[i].Last = val - buckets[i].Count-- - if buckets[i].Count < -1 { - buckets[i].Count = -1 + if buckets[i].Count > 0 { + buckets[i].Count-- } keepSnapReasons = append(keepSnapReasons, b.reason) } diff --git a/internal/restic/snapshot_policy_test.go b/internal/restic/snapshot_policy_test.go index 97a01ce03..75f0f18f4 100644 --- a/internal/restic/snapshot_policy_test.go +++ b/internal/restic/snapshot_policy_test.go @@ -22,13 +22,14 @@ func parseTimeUTC(s string) time.Time { return t.UTC() } -func parseDuration(s string) restic.Duration { - d, err := restic.ParseDuration(s) - if err != nil { - panic(err) +// Returns the maximum number of snapshots to be kept according to this policy. +// If any of the counts is -1 it will return 0. +func policySum(e *restic.ExpirePolicy) int { + if e.Last == -1 || e.Hourly == -1 || e.Daily == -1 || e.Weekly == -1 || e.Monthly == -1 || e.Yearly == -1 { + return 0 } - return d + return e.Last + e.Hourly + e.Daily + e.Weekly + e.Monthly + e.Yearly } func TestExpireSnapshotOps(t *testing.T) { @@ -46,7 +47,7 @@ func TestExpireSnapshotOps(t *testing.T) { if isEmpty != d.expectEmpty { t.Errorf("empty test %v: wrong result, want:\n %#v\ngot:\n %#v", i, d.expectEmpty, isEmpty) } - hasSum := d.p.Sum() + hasSum := policySum(d.p) if hasSum != d.expectSum { t.Errorf("sum test %v: wrong result, want:\n %#v\ngot:\n %#v", i, d.expectSum, hasSum) } @@ -219,26 +220,26 @@ func TestApplyPolicy(t *testing.T) { {Tags: []restic.TagList{{"foo"}}}, {Tags: []restic.TagList{{"foo", "bar"}}}, {Tags: []restic.TagList{{"foo"}, {"bar"}}}, - {Within: parseDuration("1d")}, - {Within: parseDuration("2d")}, - {Within: parseDuration("7d")}, - {Within: parseDuration("1m")}, - {Within: parseDuration("1m14d")}, - {Within: parseDuration("1y1d1m")}, - {Within: parseDuration("13d23h")}, - {Within: parseDuration("2m2h")}, - {Within: parseDuration("1y2m3d3h")}, - {WithinHourly: parseDuration("1y2m3d3h")}, - {WithinDaily: parseDuration("1y2m3d3h")}, - {WithinWeekly: parseDuration("1y2m3d3h")}, - {WithinMonthly: parseDuration("1y2m3d3h")}, - {WithinYearly: parseDuration("1y2m3d3h")}, - {Within: parseDuration("1h"), - WithinHourly: parseDuration("1d"), - WithinDaily: parseDuration("7d"), - WithinWeekly: parseDuration("1m"), - WithinMonthly: parseDuration("1y"), - WithinYearly: parseDuration("9999y")}, + {Within: restic.ParseDurationOrPanic("1d")}, + {Within: restic.ParseDurationOrPanic("2d")}, + {Within: restic.ParseDurationOrPanic("7d")}, + {Within: restic.ParseDurationOrPanic("1m")}, + {Within: restic.ParseDurationOrPanic("1m14d")}, + {Within: restic.ParseDurationOrPanic("1y1d1m")}, + {Within: restic.ParseDurationOrPanic("13d23h")}, + {Within: restic.ParseDurationOrPanic("2m2h")}, + {Within: restic.ParseDurationOrPanic("1y2m3d3h")}, + {WithinHourly: restic.ParseDurationOrPanic("1y2m3d3h")}, + {WithinDaily: restic.ParseDurationOrPanic("1y2m3d3h")}, + {WithinWeekly: restic.ParseDurationOrPanic("1y2m3d3h")}, + {WithinMonthly: restic.ParseDurationOrPanic("1y2m3d3h")}, + {WithinYearly: restic.ParseDurationOrPanic("1y2m3d3h")}, + {Within: restic.ParseDurationOrPanic("1h"), + WithinHourly: restic.ParseDurationOrPanic("1d"), + WithinDaily: restic.ParseDurationOrPanic("7d"), + WithinWeekly: restic.ParseDurationOrPanic("1m"), + WithinMonthly: restic.ParseDurationOrPanic("1y"), + WithinYearly: restic.ParseDurationOrPanic("9999y")}, {Last: -1}, // keep all {Last: -1, Hourly: -1}, // keep all (Last overrides Hourly) {Hourly: -1}, // keep all hourlies @@ -255,11 +256,9 @@ func TestApplyPolicy(t *testing.T) { len(keep)+len(remove), len(testExpireSnapshots)) } - if p.Last >= 0 && p.Hourly >= 0 && p.Daily >= 0 && p.Weekly >= 0 && p.Monthly >= 0 && p.Yearly >= 0 { - if p.Sum() > 0 && len(keep) > p.Sum() { - t.Errorf("not enough snapshots removed: policy allows %v snapshots to remain, but ended up with %v", - p.Sum(), len(keep)) - } + if policySum(&p) > 0 && len(keep) > policySum(&p) { + t.Errorf("not enough snapshots removed: policy allows %v snapshots to remain, but ended up with %v", + policySum(&p), len(keep)) } if len(keep) != len(reasons) {