Auto select when only one NABSL exists#193
Conversation
NicholasYancey
added
the
enhancement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCreateOptions.Complete now defers config default lookup until after the Kubernetes client and namespace are available; if StorageLocation remains unset, it lists NonAdminBackupStorageLocation in the current namespace, filters to NonAdminPhaseCreated, and auto-selects when exactly one usable NABSL exists. ChangesStorage Location Resolution
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/non-admin/backup/create.go`:
- Around line 176-189: The auto-select message is printed in Complete
(fmt.Printf in the block that sets o.StorageLocation using nabslList) which
breaks -o yaml/json output; change CreateOptions to add a boolean flag (e.g.,
storageLocationAutoSelected bool) set to true inside Complete when you
auto-select o.StorageLocation from nabslList, remove the fmt.Printf from
Complete, and then in Run—after the PrintWithFormat early return and alongside
the existing storageLocationFromConfig handling—emit the user-facing message
about auto-selection if storageLocationAutoSelected is true so the message is
suppressed for formatted output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d11b8f22-d7a8-491f-934c-6bdf1df95fed
📒 Files selected for processing (1)
cmd/non-admin/backup/create.go
| // Auto-select NABSL if exactly one exists in the namespace | ||
| if o.StorageLocation == "" { | ||
| nabslList := &nacv1alpha1.NonAdminBackupStorageLocationList{} | ||
| if err := o.client.List(context.TODO(), nabslList, &kbclient.ListOptions{ | ||
| Namespace: currentNS, | ||
| }); err != nil { | ||
| return fmt.Errorf("failed to list NonAdminBackupStorageLocations: %w", err) | ||
| } | ||
|
|
||
| if len(nabslList.Items) == 1 { | ||
| o.StorageLocation = nabslList.Items[0].Name | ||
| fmt.Printf("Auto-selected storage location: %s (only NABSL in namespace)\n", o.StorageLocation) | ||
| } | ||
| } |
There was a problem hiding this comment.
feels like you could reuse the first if o.StorageLocation == "" { from line 168
There was a problem hiding this comment.
fixed, used the same if statement instead of using 2
NicholasYancey
force-pushed
the
auto-select-nabsl
branch
2 times, most recently
from
9f48cd2 to
29f1a7d
Compare
kaovilai
left a comment
kaovilai
left a comment
There was a problem hiding this comment.
Should we add a warning that, if the user creates another non-admin BackupStorageLocation in the future, the backups may not go to the same location?
NicholasYancey
force-pushed
the
auto-select-nabsl
branch
from
29f1a7d to
93f6072
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates non-admin backup creation so the CLI can resolve a storage location automatically when the user does not pass --storage-location.
Changes:
- Adds tracking for auto-selected storage locations.
- Moves default NABSL lookup after namespace/client setup.
- Lists NABSLs in the current namespace and auto-selects when exactly one exists.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(nabslList.Items) == 1 { | ||
| o.StorageLocation = nabslList.Items[0].Name | ||
| o.storageLocationAutoSelected = true |
There was a problem hiding this comment.
This seems like a legitimate concern. We only want approved NABSLs, not pendin or rejected ones
Can we confirm that a pending/approved/rejected NABSL does not create an NABSL whatsoever to cause this issue
There was a problem hiding this comment.
fixed, checks for usable storage locations first
NicholasYancey
force-pushed
the
auto-select-nabsl
branch
from
93f6072 to
28bde6a
Compare
| // Filter to only approved/created NABSLs (exclude pending/rejected) | ||
| var usableNABSLs []nacv1alpha1.NonAdminBackupStorageLocation | ||
| for _, nabsl := range nabslList.Items { | ||
| if nabsl.Status.Phase == nacv1alpha1.NonAdminPhaseCreated { | ||
| usableNABSLs = append(usableNABSLs, nabsl) | ||
| } | ||
| } | ||
|
|
||
| if len(usableNABSLs) == 1 { | ||
| o.StorageLocation = usableNABSLs[0].Name | ||
| o.storageLocationAutoSelected = true | ||
| } |
There was a problem hiding this comment.
this is microoptimization but for consideration. no array allocation
var usableCount int
var selectedName string
for _, nabsl := range nabslList.Items {
if nabsl.Status.Phase == nacv1alpha1.NonAdminPhaseCreated {
usableCount++
if usableCount == 1 {
selectedName = nabsl.Name
} else {
break
}
}
}
if usableCount == 1 {
o.StorageLocation = selectedName
o.storageLocationAutoSelected = true
}
kaovilai
left a comment
kaovilai
left a comment
There was a problem hiding this comment.
will see if others wants this
| if len(nabslList.Items) == 1 { | ||
| o.StorageLocation = nabslList.Items[0].Name | ||
| o.storageLocationAutoSelected = true |
There was a problem hiding this comment.
This seems like a legitimate concern. We only want approved NABSLs, not pendin or rejected ones
Can we confirm that a pending/approved/rejected NABSL does not create an NABSL whatsoever to cause this issue
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joeavaikath, NicholasYancey The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Why the changes were made
there is only one valid option.
How to test the changes made
oc oadp nonadmin bsl getoc oadp nonadmin backup create test-backupExpected output:
Auto-selected storage location: (only NABSL in namespace)
NonAdminBackup request "test-backup" submitted successfully.
Summary by CodeRabbit
New Features
Bug Fixes