Nabsl enforce#29
Merged
Merged
Conversation
✨ Major improvements:
- Convert oadp.yaml to environment variable template
- Replace complex Python regex with clean envsubst
- Much more readable and maintainable approach
- Robust error handling and validation
- Template uses ${VERSION}, ${LINUX_AMD64_SHA}, etc.
🔧 Technical benefits:
- No more fragile regex string replacement
- Standard environment variable substitution
- Easier to debug and modify
- Less code, more reliable
🎯 Perfect for krew index submission: - Generate oadp.yaml directly (not oadp-final.yaml) - Release artifact is ready to drop into krew index - No renaming needed - matches krew convention exactly - Clean workflow: template → envsubst → oadp.yaml
- Removed the backup storage location (BSL) command from the non-admin command set. - Introduced a new 'get' command for listing and retrieving non-admin backups. - Updated the create command examples to include a storage location option and added a force flag for creation without specifying a storage location. - Enhanced output messages to indicate when defaults are used during backup creation.
f715afc to
5f69fbb
Compare
kaovilai
reviewed
Jul 22, 2025
kaovilai
approved these changes
Jul 22, 2025
Member
kaovilai
left a comment
There was a problem hiding this comment.
No major blockers.
Suggestions for Improvement
- Add unit tests for:
- Table formatting functions
- Age calculation logic
- Validation logic for storage location
- Get command with various scenarios - Consider adding:
- --selector flag for filtering backups in get command
- Sort options for the backup list
- More output formats beyond table/yaml/json - Documentation:
- Update README with new command examples
- Add inline comments for exported functions - Code organization:
- Extract common client setup/scheme registration into shared utilities
- Consider a common package for formatting helpers
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new get command for NonAdminBackup resources and enforces storage location validation for backup creation. The get command provides kubectl-style listing and retrieval of backups with clean table output, while the validation enhancement requires users to specify a valid NonAdminBackupStorageLocation or use a force flag for admin defaults.
- Adds new
kubectl oadp nonadmin backup getcommand with table format output - Enforces storage location validation during backup creation with --storage-location parameter
- Introduces shared client utilities for consistent scheme management across commands
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/shared/client.go | New shared utilities for client creation and namespace handling |
| cmd/non-admin/backup/get.go | New get command implementation with table formatting |
| cmd/non-admin/backup/create.go | Added storage location validation and force flag with user prompts |
| cmd/non-admin/backup/backup.go | Added get command to backup subcommand structure |
| cmd/non-admin/backup/nonadminbackup_builder.go | Removed duplicate getCurrentNamespace function |
| cmd/non-admin/backup/describe.go | Refactored to use shared client utilities |
| cmd/non-admin/backup/delete.go | Updated to use shared client creation |
| cmd/non-admin/backup/logs.go | Updated to use shared scheme utilities |
kaovilai
approved these changes
Jul 22, 2025
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
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.
Add
getcommand and enforce storage location validation for NonAdminBackupOverview
This PR adds two key enhancements to the NonAdminBackup functionality:
getcommand for listing and retrieving NonAdminBackup resourcesChanges
1. New Command:
kubectl oadp nonadmin backup getkubectl oadp nonadmin backup getkubectl oadp nonadmin backup get <backup-name>2. Enhanced Backup Create Validation
--storage-locationparameter is now mandatory--forceflag allows bypassing validation to use admin defaultsFeatures
Get Command Features
✅ Clean table output with columns:
NAME- Backup name (30 chars wide)STATUS- Current backup status from NAB phaseCREATED- Human-readable creation timestampAGE- Time since creation (e.g.,23d,15h,45m)✅ Proper integration into existing command structure
✅ Namespace-aware - automatically uses current kubectl context namespace
✅ Error handling for missing backups and empty namespaces
Validation Features
✅ Enforced best practices - requires explicit storage location reference
✅ Force flag escape hatch - allows admin override when needed
✅ Clear error messaging - guides users to provide valid NABSL or use force
✅ Updated examples - all documentation shows proper usage patterns
Usage Examples
Get Command
Create Command (Updated)
Sample Output
Get Command Output
Validation Error
Files Changed
cmd/non-admin/backup/get.go- New get command implementationcmd/non-admin/backup/backup.go- Added get command to backup subcommandcmd/non-admin/backup/create.go- Added storage location validation and force flagTesting
Get Command
Validation
Future Enhancements
Dependencies
github.com/migtools/oadp-non-adminThese enhancements improve the OADP CLI user experience by:
kubectl get-style interface for viewing NonAdminBackup resources