-
Notifications
You must be signed in to change notification settings - Fork 11
Add --all flag for nonadmin backup delete command #60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>
Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>
Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>
Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>
kaovilai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot do not add .gitignore to this PR
Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot The following files are not formatted: |
Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com>
Fixed the formatting issues by running 'go fmt ./...' on the codebase. The command formatted both files by removing trailing whitespace and ensuring proper line endings. Commit: 8fdc9a5 |
|
/hold |
|
@kaovilai this is cool and all but you can not approve the code you had ai create :) and corporations SHOULD NOT BE CONSIDERED PEOPLE :) |
Tiger approved code he generated w/ ai
|
/unhold |
|
review was already stale.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds the --all flag to the oadp nonadmin backup delete command, enabling users to delete all backups in their current namespace with a single command instead of specifying individual backup names.
- Adds a new
--allflag with proper validation to prevent conflicting usage patterns - Implements dynamic backup discovery when the
--allflag is used - Maintains full backward compatibility with existing delete functionality
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/non-admin/backup/delete.go | Core implementation of the --all flag with validation logic and backup discovery |
| cmd/non-admin/backup/delete_test.go | Comprehensive unit tests covering all usage scenarios and edge cases |
| cmd/non-admin/backup/backup_test.go | Updated test to verify the --all flag appears in command help output |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| c := &cobra.Command{ | ||
| Use: use + " NAME [NAME...]", | ||
| Use: use + " [NAME...] | --all", |
Copilot
AI
Aug 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The usage string shows [NAME...] | --all which implies mutual exclusivity using the pipe symbol, but the actual implementation allows both to be specified (though it validates against this). Consider using [NAME...] [--all] or document the mutual exclusivity more clearly in the help text.
| Use: use + " [NAME...] | --all", | |
| Use: use + " [NAME...] [--all]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lmao.. you are contradicting with your own prior feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot explain the mutual exclusivity more clearly in the help text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot explain the mutual exclusivity more clearly in the help text.
| Run: func(c *cobra.Command, args []string) { | ||
| cmd.CheckError(o.Complete(args, f)) | ||
| cmd.CheckError(o.Validate()) | ||
| cmd.CheckError(o.Validate(args)) |
Copilot
AI
Aug 19, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Validate method now requires args parameter, but this breaks the existing API contract. Consider keeping the original Validate() method signature and accessing args through the options struct or command context instead.
This PR implements the
--allflag for theoadp nonadmin backup deletecommand, allowing users to delete all backups in their current namespace with a single command.Summary
Previously, users had to specify individual backup names to delete them:
With this change, users can now delete all backups in their current namespace using:
Implementation Details
The implementation adds minimal, surgical changes to the existing delete command:
--allflag: Added toDeleteOptionsstruct and flag bindingdelete backup1 --all)--allis used, the command lists allNonAdminBackupresources in the current namespaceUsage Examples
Testing
Added comprehensive unit tests covering:
--allflagAll existing tests continue to pass, ensuring no regression in functionality.
Fixes #50.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.