Merged
Conversation
Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
📝 WalkthroughWalkthroughThis PR simplifies NABSL-request CLI flows by removing no-op validation hooks and namespace flag handling, resolving requests via direct admin-namespace Get/List lookup, improving describe output formatting (alignment, color, indented maps), adding shared output helpers, pruning indirect go.mod deps, removing several root subcommands, and adding a Makefile GOOS validation. Changes
Sequence Diagram(s)sequenceDiagram
actor CLI as CLI Client
participant Finder as FindNABSLRequestByNameOrUUID
participant API as API Server (admin namespace)
participant Formatter as Describe Output Formatter
CLI->>Finder: Resolve input (name or UUID)
Finder->>API: Get request by name/UUID in admin ns
alt Get succeeds
API-->>Finder: Return request
else NotFound
Finder->>API: List requests in admin ns
API-->>Finder: Return list
Finder->>Finder: Match on Status.SourceNonAdminBSL.Name
end
Finder-->>CLI: Return canonical request name
CLI->>API: Get full request details by canonical name
API-->>CLI: Return request object
CLI->>Formatter: Format description
Formatter->>Formatter: Colorize phase, indent, print labels/config aligned
Formatter-->>CLI: Rendered output
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
Signed-off-by: Joseph <jvaikath@redhat.com>
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/shared/nabsl_requests.go (1)
36-50:⚠️ Potential issue | 🟠 MajorReturn non-NotFound errors from the UUID lookup instead of silently falling back.
Currently, any
Geterror triggers a list fallback, which masks auth, network, and permission failures—allowing the function to surface a misleading "request not found" error at the end when the real problem was elsewhere.Only proceed to the fallback when the error is explicitly a NotFound error. For all other errors, return immediately with proper context.
🔧 Suggested fix
import ( "context" "fmt" kbclient "sigs.k8s.io/controller-runtime/pkg/client" + "k8s.io/apimachinery/pkg/api/errors" nacv1alpha1 "github.com/migtools/oadp-non-admin/api/v1alpha1" ) @@ err := client.Get(ctx, kbclient.ObjectKey{ Name: nameOrUUID, Namespace: adminNamespace, }, &testRequest) if err == nil { return nameOrUUID, nil } + if !errors.IsNotFound(err) { + return "", fmt.Errorf("failed to get request %q: %w", nameOrUUID, err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/shared/nabsl_requests.go` around lines 36 - 50, The UUID lookup currently treats any error from client.Get(ctx, kbclient.ObjectKey{Name: nameOrUUID, Namespace: adminNamespace}, &testRequest) as a signal to fallback to listing; change this so only a NotFound error falls through to the list path—if client.Get returns a non-NotFound error (e.g. network/auth/permission error) return that error immediately with context (wrap/format it) instead of proceeding; keep the fallback behavior when the error is explicitly a NotFound (use the appropriate IsNotFound helper for your imports) and then continue with the existing client.List logic using requestList and adminNamespace.
🧹 Nitpick comments (1)
cmd/nabsl-request/approve.go (1)
87-89: Minor: Remove leftover empty line.Line 88 appears to be a leftover from removing the
Validate()call. Consider removing the empty line to keep the function body clean.🔧 Suggested cleanup
func (o *ApproveOptions) Run(c *cobra.Command, f client.Factory) error { - adminNS := f.Namespace()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/nabsl-request/approve.go` around lines 87 - 89, The function ApproveOptions.Run contains an extra blank line between the variable assignment adminNS := f.Namespace() and surrounding code; remove that leftover empty line so the function body is compact and the adminNS assignment is directly adjacent to the next statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/nabsl-request/describe.go`:
- Around line 108-184: The code dereferences spec.ObjectStorage without checking
for nil (when reading spec.ObjectStorage.Bucket and spec.ObjectStorage.Prefix),
which can panic for partial/invalid requests; update the block that handles
source.RequestedSpec (the Requested BackupStorageLocation Spec logic for
variable spec := source.RequestedSpec) to first check if spec.ObjectStorage !=
nil before accessing its fields and only print "Object Storage Bucket" and
"Prefix" when ObjectStorage is non-nil; retain printing other spec fields as-is
and ensure the nil check guards both Bucket and Prefix accesses.
---
Outside diff comments:
In `@cmd/shared/nabsl_requests.go`:
- Around line 36-50: The UUID lookup currently treats any error from
client.Get(ctx, kbclient.ObjectKey{Name: nameOrUUID, Namespace: adminNamespace},
&testRequest) as a signal to fallback to listing; change this so only a NotFound
error falls through to the list path—if client.Get returns a non-NotFound error
(e.g. network/auth/permission error) return that error immediately with context
(wrap/format it) instead of proceeding; keep the fallback behavior when the
error is explicitly a NotFound (use the appropriate IsNotFound helper for your
imports) and then continue with the existing client.List logic using requestList
and adminNamespace.
---
Nitpick comments:
In `@cmd/nabsl-request/approve.go`:
- Around line 87-89: The function ApproveOptions.Run contains an extra blank
line between the variable assignment adminNS := f.Namespace() and surrounding
code; remove that leftover empty line so the function body is compact and the
adminNS assignment is directly adjacent to the next statement.
Signed-off-by: Joseph <jvaikath@redhat.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/shared/nabsl_requests.go (1)
43-63:⚠️ Potential issue | 🔴 CriticalCritical logic bug: Early return on NotFound blocks NABSL name lookup.
The function documentation (lines 29-31) explicitly states it supports two lookup strategies: direct UUID lookup and NABSL name lookup. However, the early return at lines 45-46 when
errors.IsNotFound(err)prevents the fallback List operation from executing when a user provides a NABSL name.When a user provides a NABSL name, the Get call returns NotFound (since request objects are named by UUID), triggering the return at line 46. This bypasses the List operation at lines 49-61 that searches by
Status.SourceNonAdminBSL.Name, breaking the documented behavior that all four callers (describe, approve, reject, get) depend on.The condition at line 45 should be inverted to
!errors.IsNotFound(err)so that NotFound errors fall through to the List fallback while actual errors are properly reported.🐛 Proposed fix
if err == nil { return nameOrUUID, nil - } else if errors.IsNotFound(err) { - return "", fmt.Errorf("request for NABSL %q not found", nameOrUUID) + } else if !errors.IsNotFound(err) { + // Non-NotFound errors (e.g., network issues) should be reported + return "", fmt.Errorf("failed to get request: %w", err) } - // Fallback:Match NABSL name to UUID + // Fallback: Match NABSL name to UUID (also handles case where nameOrUUID wasn't a valid UUID) var requestList nacv1alpha1.NonAdminBackupStorageLocationRequestList err = client.List(ctx, &requestList, kbclient.InNamespace(adminNamespace))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/shared/nabsl_requests.go` around lines 43 - 63, The lookup currently bails out on errors.IsNotFound(err) and thus never runs the fallback List search; in the function that resolves nameOrUUID (where client.List, ctx, adminNamespace and variable requestList of type nacv1alpha1.NonAdminBackupStorageLocationRequestList are used), change the conditional so that you return immediately only on success (err == nil) or on any real error (i.e., when err != nil && !errors.IsNotFound(err)), but allow NotFound to fall through to the List-based fallback that scans request.Status.SourceNonAdminBSL.Name and returns the matching request.Name; this preserves direct UUID success, reports real errors, and lets NABSL-name lookups proceed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/shared/nabsl_requests.go`:
- Around line 43-63: The lookup currently bails out on errors.IsNotFound(err)
and thus never runs the fallback List search; in the function that resolves
nameOrUUID (where client.List, ctx, adminNamespace and variable requestList of
type nacv1alpha1.NonAdminBackupStorageLocationRequestList are used), change the
conditional so that you return immediately only on success (err == nil) or on
any real error (i.e., when err != nil && !errors.IsNotFound(err)), but allow
NotFound to fall through to the List-based fallback that scans
request.Status.SourceNonAdminBSL.Name and returns the matching request.Name;
this preserves direct UUID success, reports real errors, and lets NABSL-name
lookups proceed.
kaovilai
reviewed
Feb 24, 2026
kaovilai
approved these changes
Feb 25, 2026
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
2 participants
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.
Why the changes were made
make installrunsHow to test the changes made
oc oadp nabsl-request describeshould now work correctlySummary by CodeRabbit
New Features
Bug Fixes
Changes
Tests