Prompts user to choose when more than one NABSL exists#198
Prompts user to choose when more than one NABSL exists#198NicholasYancey wants to merge 7 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: NicholasYancey The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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)
📝 WalkthroughWalkthroughThe PR refactors non-admin backup storage location handling in ChangesInteractive NonAdminBackupStorageLocation Selection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 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 316-340: The interactive prompt sends its menu to the provided out
writer (and the call site passes os.Stdout) but only checks stdin for a TTY, so
prompts can be written into redirected stdout; update promptForNABSLSelection
(and the call that passes os.Stdout) to either write interactive UI to stderr
(use os.Stderr when in a terminal) or first verify that out is a terminal and
refuse to prompt otherwise; specifically change the menu/Prompt writes
(fmt.Fprintln/Fprintf to out) to target the terminal stream (stderr) or add a
terminal check on out (term.IsTerminal on out's FD) and return an error if not a
TTY so prompting is never written into redirected stdout. Ensure references:
promptForNABSLSelection, the caller that passes os.Stdout, and the menu printing
lines are updated accordingly.
- Around line 305-323: The function resolveStorageLocationFromList should first
filter the incoming items slice to only include those with Status.Phase ==
nacv1alpha1.NonAdminPhaseCreated, then make decisions from that filtered slice:
if the filtered slice is empty return an explicit error, if it has exactly one
set o.StorageLocation and o.storageLocationAutoSelected = true, and if it has
more than one call promptForNABSLSelection to set o.StorageLocation and
o.storageLocationPrompted = true; update control flow so you never prompt with
unusable NABSLs and you fail early when no usable NABSLs exist.
🪄 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: aad0fde4-186e-442c-9065-4a8adc482db6
📒 Files selected for processing (1)
cmd/non-admin/backup/create.go
There was a problem hiding this comment.
Pull request overview
This PR improves the non-admin backup creation flow by adding an interactive selection prompt for --storage-location when multiple NonAdminBackupStorageLocations (NABSLs) exist, reducing the need for users to know/set the location name ahead of time.
Changes:
- Adds terminal-based prompting to choose a NABSL when more than one is present.
- Refactors storage-location resolution into helper methods and adds user feedback for prompted vs auto-selected cases.
- Adds
golang.org/x/termas a direct dependency for terminal detection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
go.mod |
Adds golang.org/x/term as a direct dependency to support interactive terminal prompting. |
cmd/non-admin/backup/create.go |
Implements interactive NABSL selection and refactors storage-location resolution/output messaging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i, nabsl := range items { | ||
| fmt.Fprintf(out, " %d) %s (%s)\n", i+1, nabsl.Name, formatNABSLPhase(&nabsl)) | ||
| } |
consistent with pr migtools#193, only uses usable nabsl Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/non-admin/backup/create.go (1)
315-317:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn an explicit error when no usable NABSL exists.
When
usableis empty, returningnilfalls through to a later generic--storage-location is requirederror instead of reporting that noCreatedNABSL is currently usable in the namespace.Suggested fix
switch len(usable) { case 0: - return nil + return fmt.Errorf("no usable NonAdminBackupStorageLocations found; create one or wait until one reaches Created, or specify --storage-location explicitly") case 1:🤖 Prompt for 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. In `@cmd/non-admin/backup/create.go` around lines 315 - 317, The switch handling the length of the usable slice currently returns nil for case 0 which masks the real problem; change the case 0 branch to return an explicit error (e.g., using fmt.Errorf) that states no usable NABSL exists/has phase "Created" in the target namespace so callers see a clear message instead of the later generic "--storage-location is required" error; update the case 0 in the function that inspects the usable variable (and reference the namespace/name variables used nearby) to return that descriptive error.
🤖 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`:
- Line 331: There is an extra unmatched closing brace (`}`) at the end of the
file that causes a compilation error; remove that trailing `}` so the function
and file braces are balanced (inspect the end of the create.go source around the
end of the command constructor/handler such as the Create command or its RunE
function to confirm proper brace pairing and delete the dangling brace).
---
Duplicate comments:
In `@cmd/non-admin/backup/create.go`:
- Around line 315-317: The switch handling the length of the usable slice
currently returns nil for case 0 which masks the real problem; change the case 0
branch to return an explicit error (e.g., using fmt.Errorf) that states no
usable NABSL exists/has phase "Created" in the target namespace so callers see a
clear message instead of the later generic "--storage-location is required"
error; update the case 0 in the function that inspects the usable variable (and
reference the namespace/name variables used nearby) to return that descriptive
error.
🪄 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: 9d7c3f6a-ebb1-4b19-a94e-418440275c55
📒 Files selected for processing (2)
cmd/non-admin/backup/create.gogo.mod
Why the changes were made
PR #193 added auto-selection when exactly one usable NABSL exists in the namespace. That removed the need to pass --storage-location or set default-nabsl in that case.
When a user has two or more NABSLs in their namespace, the CLI still failed with --storage-location is required error. Users had to know the location name up front or set a default in config. This change helps with if there is more than one NABSL and allows the user to pick one that they want instead of giving an error.
How to test the changes made
Build and install
Test (2+ NABSLs)
bsl get should show 2 or more NABSLs before running backup create.
You should see a numbered NABSL list, then after picking one:
Confirm with:
Summary by CodeRabbit
New Features
Chores