feat(mpg): add --pitr-time to restore - #5051
Merged
Merged
Conversation
ui-ex just shipped point-in-time restore (pitr_time) as an alternative to --backup-id across its v2/dashboard and public Flaps restore APIs. Adds the matching client flag: exactly one of --backup-id/--pitr-time is required, and v1 clusters -- which don't support PITR at all, a deliberate product decision -- are rejected client-side with the same error message the v1 API itself returns, instead of round-tripping to get a 422.
An independent review of the --pitr-time flag flagged the new one-of/mutually-exclusive/v1-rejection validation and the v2 JSON serialization as untested. Adds: - runRestore coverage for: neither flag, both flags, v1 rejecting pitr_time (asserting no restore call is issued), and v1/v2 dispatch for both backup_id and pitr_time. - a serialization test proving RestoreClusterBackupInput's omitempty actually drops the unused field from the JSON body, not just that the Go struct field is zero-valued.
Follow-up from an independent review of the --pitr-time flag, refined by a second independent pass: - --pitr-time is now parsed as RFC3339 and rejected before any network call for malformed or timezone-less input. The parsed value is not used for anything else -- the original string is still forwarded to the backend unchanged (it already normalizes offsets to UTC server-side; validating-then-forwarding-raw avoids two independent transforms that would need to agree on fractional-second handling). - one-of/mutually-exclusive/RFC3339-format checks now run before the cluster lookup, since none of them need cluster info. The v1-PITR-rejection check stays after the lookup -- it needs cluster.Version. - restore's help text and generic restore-failure error messages no longer say "backup" unconditionally, now that a restore target can be a backup, a point in time, or (via #5044) neither with a bare name override. - tests: malformed/timezone-less --pitr-time rejected before any client call, a non-UTC-offset value forwarded unchanged (regression guard against re-introducing client-side normalization later), and name+pitr_time proven to coexist correctly in the v2 wire format.
tripledoublev
force-pushed
the
vincent/mpg-restore-pitr-flag
branch
from
August 7, 2026 14:09
5a98028 to
a75c74d
Compare
Pre-existing lint violation on master, unrelated to this branch's own changes, surfaced by CI's full-repo lint pass. One-line fix: blank line before the trailing return.
tripledoublev
marked this pull request as ready for review
August 7, 2026 18:30
jwnx
reviewed
Aug 7, 2026
Comment on lines
41
to
+69
| Shorthand: "n", | ||
| Description: "The name of the restored cluster (defaults to a generated name)", | ||
| }, | ||
| flag.String{ | ||
| Name: "pitr-time", | ||
| Description: "Restore to a specific point in time (RFC3339, e.g. 2026-06-01T12:00:00Z). Requires the cluster's PITR recovery window to cover this time. Mutually exclusive with --backup-id.", | ||
| }, | ||
| ) | ||
|
|
||
| return cmd | ||
| } | ||
|
|
||
| func runRestore(ctx context.Context) error { | ||
| clusterID := flag.FirstArg(ctx) | ||
| backupID := flag.GetString(ctx, "backup-id") | ||
| pitrTime := flag.GetString(ctx, "pitr-time") | ||
| name := flag.GetString(ctx, "name") | ||
| if backupID == "" && pitrTime == "" { | ||
| return fmt.Errorf("one of --backup-id or --pitr-time is required") | ||
| } | ||
| if backupID != "" && pitrTime != "" { | ||
| return fmt.Errorf("--backup-id and --pitr-time are mutually exclusive") | ||
| } | ||
| if pitrTime != "" { | ||
| if _, err := time.Parse(time.RFC3339, pitrTime); err != nil { | ||
| return fmt.Errorf("--pitr-time must be an RFC3339 timestamp with an explicit offset (e.g. 2026-06-01T12:00:00Z): %w", err) | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
You can use Cobra to check for flags and make sure they're mutually exclusive / required.
Maybe something like...
cmd.MarkFlagRequired("name")
cmd.MarkFlagsOneRequired("pitr-time", "backup")
cmd.MarkFlagsMutuallyExclusive("pitr-time","backup")(don't remember the syntax, bad memory)
Contributor
Author
There was a problem hiding this comment.
Thanks! Looked into Cobra's flag-grouping helpers and noticed they're not used anywhere in the codebase yet, so this'd be introducing a new pattern rather than following an existing one.
I think this could be a nice follow-up PR though, maybe worth applying more broadly across mpg commands, rather than as a one-off here.
Contributor
There was a problem hiding this comment.
maybe worth applying more broadly across mpg commands
yes!
tripledoublev
changed the title
--pitr-time to restore
jwnx
approved these changes
Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--pitr-time <RFC3339_TIMESTAMP>tofly mpg restore. Exactly one of--backup-idor--pitr-timeis required.--pitr-timefor v1 clusters client-side.pitr_timeonly through the v2 restore request.Test plan
go test ./internal/command/mpg/... ./internal/uiex/mpg/v1/... ./internal/uiex/mpg/v2/...go vet ./...gofmt -l .Live-tested against fra-staging
Built this branch and pointed it at the v2 staging API, against an MPG v2 cluster on with timestamped test rows inserted directly via
psql:--pitr-time+--nametogether — succeeded, correct cluster name used--pitr-timealone (no--name) — succeeded, default name uses the dash convention (cluster-restored-YYYYMMDD-HHMM)--backup-id+--nametogether — succeeded (regression check: confirmed unaffected by the PITR changes)--backup-idalone (no--name) — succeeded, same dash-convention default name--pitr-time— rejected client-side with a clear RFC3339 error, no network call--backup-id+--pitr-timetogether — rejected client-side as mutually exclusiveDependencies
Stacked on #5044. The ui-ex v2 PITR API must be merged and deployed before this is released.