PBM-1600 - Interactive confirm prompt when restore#1180
PBM-1600 - Interactive confirm prompt when restore#1180littlecxm wants to merge 2 commits intopercona:devfrom
Conversation
|
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements an interactive confirmation prompt for restore operations, aligning with the JIRA ticket PBM-1600. The change adds a safety mechanism requiring user confirmation before executing potentially destructive restore operations.
Key changes:
- Extracted confirmation prompt functionality from delete command to a reusable utility
- Added interactive confirmation prompt to restore operations with bypass option
- Improved console output formatting for restore operations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pbm/util/cmd_prompt.go | New utility function for interactive confirmation prompts with TTY detection and colored output |
| pbm/errors/errors.go | Added standard user cancellation error for consistent error handling |
| cmd/pbm/restore.go | Integrated confirmation prompt into restore workflow with bypass flag and improved output formatting |
| cmd/pbm/delete.go | Refactored to use shared confirmation utility and standardized error handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| nsFrom string | ||
| nsTo string | ||
| usersAndRoles bool | ||
| confirmYes bool |
There was a problem hiding this comment.
The confirmYes field is added to the struct but there's no visible command-line flag definition or initialization code in this diff. Ensure that a corresponding CLI flag (like --yes or --confirm) is properly defined and wired to this field.
| } | ||
|
|
||
| if runtime.GOOS == "linux" { | ||
| question = fmt.Sprintf("\033[1;33m%s\033[0m", question) // Yellow text for Linux terminals |
There was a problem hiding this comment.
The color formatting is only applied to Linux systems, but many other Unix-like systems (macOS, FreeBSD, etc.) also support ANSI color codes. Consider using a more inclusive condition like runtime.GOOS != "windows" or detecting terminal color support more robustly.
| question = fmt.Sprintf("\033[1;33m%s\033[0m", question) // Yellow text for Linux terminals | |
| if runtime.GOOS != "windows" { | |
| question = fmt.Sprintf("\033[1;33m%s\033[0m", question) // Yellow text for Unix-like terminals |
There was a problem hiding this comment.
I'd drop this block for two reasons
- We don't use colours anywhere
- On linux it would print the question twice ;)
| "bufio" | ||
| "context" | ||
| "fmt" | ||
| "github.com/percona/percona-backup-mongodb/pbm/util" |
There was a problem hiding this comment.
The import should follow Go conventions by being grouped with other third-party imports and maintaining alphabetical order. Move this import to be grouped with other project imports.
| "github.com/percona/percona-backup-mongodb/pbm/util" |
| return nil, errors.Wrap(err, "send command") | ||
| } | ||
|
|
||
| fmt.Printf("Starting restore") |
There was a problem hiding this comment.
The message "Starting restore" lacks context compared to the original message that included the backup name and other details. Consider including key restore information in this status message for better user experience.
| fmt.Printf("Starting restore") | |
| fmt.Printf("Starting restore: %s%s%s\n", name, pitrs, bcpName) |
|
@littlecxm Hi, thank you for the PR. Could I please ask you to rebase against latest dev and address the comments? |
3 participants
Hi!
I'm submitting PR to add implement in PBM-1600.
I have tested this prompt on my env (debian-amd64).
Thanks a lot for this awesome project and maintainer!