verb-noun order bugfixes#118
Merged
Merged
Conversation
Signed-off-by: Joseph <jvaikath@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughRemoved the dynamic non-admin verb builder/registry and replaced it with explicit Cobra verb commands; added a local non-admin output package; gated extended backup describe output behind a Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant User
end
rect rgba(200,255,200,0.5)
participant CLI_Verb_Command
participant Noun_Subcommand
end
rect rgba(255,230,200,0.5)
participant Output_Package
participant Kubernetes_API
end
User->>CLI_Verb_Command: run "oadp nonadmin <verb> <noun> [flags]"
CLI_Verb_Command->>Noun_Subcommand: invoke specific noun subcommand (backup/restore/bsl)
Noun_Subcommand->>Kubernetes_API: fetch resource(s) per args/flags
Kubernetes_API-->>Noun_Subcommand: return resource object(s)
alt --details flag set (describe)
Noun_Subcommand->>Noun_Subcommand: fetch & format extended details
end
Noun_Subcommand->>Output_Package: PrintWithFormat(obj, formatFlags)
Output_Package-->>User: formatted output (yaml/json/table)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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
🤖 Fix all issues with AI agents
In `@cmd/non-admin/verbs/builder_test.go`:
- Around line 408-410: The test currently only logs when err.Error() does not
contain "command not found" which means it never fails on unexpected error
messages; update the assertion in the test (in builder_test.go around the
strings.Contains(err.Error(), "command not found") check) to fail the test when
the error message is not what we expect by replacing the log with a test failure
(e.g., call t.Fatalf or t.Errorf) that includes the actual err.Error() so the
test fails and reports the unexpected message.
🧹 Nitpick comments (5)
cmd/non-admin/backup/describe.go (1)
325-344: Redundant context creation insideprintNonAdminBackupDetails.A new context with timeout is created at lines 327-328, but the caller at line 38 already creates a context with the same timeout. Consider passing the existing context as a parameter instead of creating a new one. This would:
- Avoid redundant timeout tracking
- Ensure proper cancellation propagation from parent context
- Follow the convention of accepting context as first parameter
♻️ Suggested approach
Pass
ctxfrom the caller intoprintNonAdminBackupDetailsinstead of creating a new context:-func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBackup, kbClient kbclient.Client, backupName string, userNamespace string, timeout time.Duration, showDetails bool) { +func printNonAdminBackupDetails(ctx context.Context, cmd *cobra.Command, nab *nacv1alpha1.NonAdminBackup, kbClient kbclient.Client, backupName string, userNamespace string, timeout time.Duration, showDetails bool) {Then in the
showDetailsblock, use the passed context directly:if showDetails { - ctx, cancel := context.WithTimeout(context.Background(), timeout) - defer cancel() - resourceList, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{Update the call site at line 75:
- printNonAdminBackupDetails(cmd, &nab, kbClient, backupName, userNamespace, effectiveTimeout, details) + printNonAdminBackupDetails(ctx, cmd, &nab, kbClient, backupName, userNamespace, effectiveTimeout, details)cmd/non-admin/verbs/builder.go (1)
65-84: Consider usingstrings.Joinand sorting for deterministic output.The current implementation uses manual string concatenation and doesn't sort the resource types. While acceptable for error messages, this could produce inconsistent output order across runs.
♻️ Suggested improvement
+import ( + "sort" + "strings" + ... +) func (vb *NonAdminVerbBuilder) getAvailableResourceTypes() string { if len(vb.resourceRegistry) == 0 { return "none" } var types []string for resourceType := range vb.resourceRegistry { types = append(types, resourceType) } + sort.Strings(types) - result := "" - for i, t := range types { - if i > 0 { - result += ", " - } - result += t - } - return result + return strings.Join(types, ", ") }cmd/non-admin/verbs/builder_test.go (2)
451-455: Consider asserting the duration value instead of just logging.The test verifies the flag was received but doesn't assert the normalized value. If the duration normalization changes or breaks, the test would still pass while only logging the discrepancy.
Proposed fix for stricter validation
if !flagReceived { t.Error("Expected duration flag --ttl to be passed to delegated command") - } else if receivedValue != "720h0m0s" { - t.Logf("Duration flag passed with value: %s (expected normalized form)", receivedValue) + } else if receivedValue != "720h0m0s" { + t.Errorf("Expected duration value '720h0m0s', got: %s", receivedValue) + } else { + t.Logf("Duration flag passed with expected value: %s", receivedValue) }
498-505: Map contents not validated against expected values.The test verifies the flag was received but doesn't assert that
receivedMapcontains the expected entries (region=us-east-1andbucket=my-bucket). This reduces the test's ability to catch regressions in map flag parsing.Proposed fix to validate map contents
if !flagReceived { t.Error("Expected map flag --config to be passed to delegated command") } else { - t.Logf("Map flag passed successfully with %d entries", len(receivedMap)) - if len(receivedMap) > 0 { - t.Logf("Map contents: %v", receivedMap) + if len(receivedMap) != 2 { + t.Errorf("Expected 2 map entries, got %d: %v", len(receivedMap), receivedMap) + } + if receivedMap["region"] != "us-east-1" { + t.Errorf("Expected region=us-east-1, got region=%s", receivedMap["region"]) + } + if receivedMap["bucket"] != "my-bucket" { + t.Errorf("Expected bucket=my-bucket, got bucket=%s", receivedMap["bucket"]) } + t.Logf("Map flag passed with expected contents: %v", receivedMap) }cmd/non-admin/README.md (1)
220-245: Add language specifier to fenced code blocks.The ASCII flow diagrams should have a language specifier (e.g.,
textorplaintext) to satisfy markdown linting rules.Proposed fix
-``` +```text User: kubectl oadp nonadmin backup create my-backup --storage-location aws Flow: 1. NonadminCommand 2. → BackupCommand 3. → CreateCommand 4. → Flags parsed directly on CreateCommand 5. → CreateCommand.RunE() executes```diff -``` +```text User: kubectl oadp nonadmin create backup my-backup --storage-location aws Flow: 1. NonadminCommand 2. → CreateCommand (verb) 3. → Extract resource type: "backup" 4. → Create fresh backup command instance 5. → Get "create" subcommand from backup 6. → Convert verb flags to args 7. → Parse args on fresh subcommand 8. → Execute subcommand.RunE()</details> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/non-admin/verbs/builder.go`:
- Around line 168-172: The boolean branch only appends the flag when flagValue
== "true", so explicit false values (e.g. --details=false) are lost; update the
case "bool" in builder.go to always propagate the user's explicit value: if
flagValue == "true" append "--"+flag.Name (preserving shorthand form) otherwise
append "--"+flag.Name+"="+flagValue so that false (or any explicit boolean) is
passed through via remainingArgs.
- Around line 173-181: The stringArray/stringSlice branch currently appends the
entire bracketed String() output as one value causing a nested array on
re-parse; change the logic in the case "stringArray", "stringSlice" to detect
and handle bracket-enclosed values: if flagValue starts with "[" and ends with
"]", trim the brackets, split the inner string on commas, trim whitespace from
each item, and for each non-empty item append "--"+flag.Name and the item to
remainingArgs (so repeated flags are passed); otherwise keep the existing
single-value append behavior for non-bracket values. Ensure you continue to
check flagValue != "" && flagValue != "[]".
🧹 Nitpick comments (1)
cmd/non-admin/verbs/builder.go (1)
65-84: Consider usingstrings.Joinand sorting for consistency.The manual string concatenation can be simplified with
strings.Join. Additionally, map iteration order in Go is non-deterministic, so error messages may show resource types in different orders across executions. Sorting provides consistent output.♻️ Suggested refactor
+import ( + "sort" + "strings" + ... +) func (vb *NonAdminVerbBuilder) getAvailableResourceTypes() string { if len(vb.resourceRegistry) == 0 { return "none" } var types []string for resourceType := range vb.resourceRegistry { types = append(types, resourceType) } - - result := "" - for i, t := range types { - if i > 0 { - result += ", " - } - result += t - } - return result + sort.Strings(types) + return strings.Join(types, ", ") }
Comment on lines
+173
to
+181
| case "stringArray", "stringSlice": | ||
| // Handle string array/slice flags | ||
| remainingArgs = append(remainingArgs, "--"+flag.Name, flagValue) | ||
| // For array/slice flags, we need to preserve multiple values | ||
| // The String() representation might be comma-separated or bracket-enclosed | ||
| // For pflag arrays, each value should be passed with its own flag instance | ||
| // However, since we're re-parsing, the single string representation should work | ||
| // as pflag will handle splitting appropriately | ||
| if flagValue != "" && flagValue != "[]" { | ||
| remainingArgs = append(remainingArgs, "--"+flag.Name, flagValue) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and examine the actual code
find . -name "builder.go" -path "*/non-admin/verbs/*" | head -5Repository: migtools/oadp-cli
Length of output: 93
🏁 Script executed:
# Check if the test file exists
find . -name "builder_test.go" -path "*/non-admin/verbs/*" | head -5Repository: migtools/oadp-cli
Length of output: 98
🏁 Script executed:
# List the repository structure to understand the project
ls -laRepository: migtools/oadp-cli
Length of output: 1272
🏁 Script executed:
# Examine the builder.go file around lines 173-181
cat -n cmd/non-admin/verbs/builder.go | sed -n '165,190p'Repository: migtools/oadp-cli
Length of output: 1408
🏁 Script executed:
# Check the broader context to understand the addFlagsToArgs function
cat -n cmd/non-admin/verbs/builder.go | sed -n '140,200p'Repository: migtools/oadp-cli
Length of output: 2881
🏁 Script executed:
# Look at the test file to see current test coverage
cat -n cmd/non-admin/verbs/builder_test.go | sed -n '70,185p'Repository: migtools/oadp-cli
Length of output: 4188
🏁 Script executed:
# Create a test to verify how pflag actually parses stringArray with bracket-enclosed values
cd /tmp && cat > test_pflag_parse.go << 'EOF'
package main
import (
"fmt"
"strings"
"github.com/spf13/pflag"
)
func main() {
tests := []struct {
name string
args []string
desc string
}{
{
name: "bracket-enclosed",
args: []string{"--items", "[a,b,c]"},
desc: "pflag with bracket-enclosed string",
},
{
name: "unbracketed-comma",
args: []string{"--items", "a,b,c"},
desc: "pflag with unbracketed comma-separated",
},
{
name: "repeated-flags",
args: []string{"--items", "a", "--items", "b", "--items", "c"},
desc: "pflag with repeated flags",
},
}
for _, test := range tests {
fmt.Printf("\n=== Test: %s (%s) ===\n", test.name, test.desc)
fmt.Printf("Input args: %v\n", test.args)
fs := pflag.NewFlagSet("test", pflag.ContinueOnError)
var items []string
fs.StringArrayVar(&items, "items", nil, "test items")
err := fs.Parse(test.args)
if err != nil {
fmt.Printf("Parse error: %v\n", err)
continue
}
fmt.Printf("Parsed array: %v\n", items)
fmt.Printf("Array length: %d\n", len(items))
// Simulate what String() returns and re-parse
if len(items) > 0 {
// Manually construct what String() would return
stringRepr := "[" + strings.Join(items, ",") + "]"
fmt.Printf("\nSimulating re-parse with String() output: %q\n", stringRepr)
fs2 := pflag.NewFlagSet("test2", pflag.ContinueOnError)
var items2 []string
fs2.StringArrayVar(&items2, "items", nil, "test items")
err2 := fs2.Parse([]string{"--items", stringRepr})
if err2 != nil {
fmt.Printf("Re-parse error: %v\n", err2)
} else {
fmt.Printf("Re-parsed array: %v\n", items2)
fmt.Printf("Re-parsed length: %d\n", len(items2))
if len(items2) == 1 && items2[0] == stringRepr {
fmt.Printf("WARNING: Re-parse treated entire bracket string as single element!\n")
}
}
}
}
}
EOF
go mod init test 2>/dev/null
go mod edit -require=github.com/spf13/pflag@latest 2>/dev/null
go mod tidy 2>&1 | head -5
go run test_pflag_parse.goRepository: migtools/oadp-cli
Length of output: 454
🏁 Script executed:
# Use the repo's go.mod to run our test
cat > cmd/non-admin/verbs/test_pflag_parse.go << 'EOF'
package verbs
import (
"fmt"
"strings"
"testing"
"github.com/spf13/pflag"
)
func TestPflagStringArrayParsing(t *testing.T) {
tests := []struct {
name string
args []string
desc string
}{
{
name: "bracket-enclosed",
args: []string{"--items", "[a,b,c]"},
desc: "pflag with bracket-enclosed string",
},
{
name: "unbracketed-comma",
args: []string{"--items", "a,b,c"},
desc: "pflag with unbracketed comma-separated",
},
{
name: "repeated-flags",
args: []string{"--items", "a", "--items", "b", "--items", "c"},
desc: "pflag with repeated flags",
},
}
for _, test := range tests {
fmt.Printf("\n=== Test: %s (%s) ===\n", test.name, test.desc)
fmt.Printf("Input args: %v\n", test.args)
fs := pflag.NewFlagSet("test", pflag.ContinueOnError)
var items []string
fs.StringArrayVar(&items, "items", nil, "test items")
err := fs.Parse(test.args)
if err != nil {
fmt.Printf("Parse error: %v\n", err)
continue
}
fmt.Printf("Parsed array: %v\n", items)
fmt.Printf("Array length: %d\n", len(items))
// Simulate what String() returns and re-parse
if len(items) > 0 {
// Manually construct what String() would return
stringRepr := "[" + strings.Join(items, ",") + "]"
fmt.Printf("\nSimulating re-parse with String() output: %q\n", stringRepr)
fs2 := pflag.NewFlagSet("test2", pflag.ContinueOnError)
var items2 []string
fs2.StringArrayVar(&items2, "items", nil, "test items")
err2 := fs2.Parse([]string{"--items", stringRepr})
if err2 != nil {
fmt.Printf("Re-parse error: %v\n", err2)
} else {
fmt.Printf("Re-parsed array: %v\n", items2)
fmt.Printf("Re-parsed length: %d\n", len(items2))
if len(items2) == 1 && items2[0] == stringRepr {
fmt.Printf("WARNING: Re-parse treated entire bracket string as single element!\n")
}
}
}
}
}
EOF
# Now run the test from the repository root
cd . && go test -v -run TestPflagStringArrayParsing ./cmd/non-admin/verbs/Repository: migtools/oadp-cli
Length of output: 5221
🏁 Script executed:
# Also let's check what the actual String() output looks like for stringArray flags in this codebase
grep -n "StringArray\|stringArray" cmd/non-admin/verbs/*.go | head -20Repository: migtools/oadp-cli
Length of output: 789
🏁 Script executed:
# Let me create a simpler standalone test that can run directly
cat > /tmp/test_array_parse.go << 'EOF'
package main
import (
"fmt"
"strings"
"github.com/spf13/pflag"
)
func main() {
fmt.Println("Testing pflag stringArray parsing behavior:")
fmt.Println("===========================================")
// Test 1: What does String() return for a parsed stringArray?
fmt.Println("\n[Test 1] Initial parse and String() output:")
fs1 := pflag.NewFlagSet("test1", pflag.ContinueOnError)
var items1 []string
fs1.StringArrayVar(&items1, "items", nil, "test items")
fs1.Parse([]string{"--items", "a", "--items", "b", "--items", "c"})
fmt.Printf(" Parsed with repeated flags: %v\n", items1)
// Get the flag and call String() on it
flag1 := fs1.Lookup("items")
stringRepr := flag1.Value.String()
fmt.Printf(" String() output: %q\n", stringRepr)
// Test 2: Re-parse the bracket-enclosed output
fmt.Println("\n[Test 2] Re-parsing the String() output (bracket-enclosed):")
fs2 := pflag.NewFlagSet("test2", pflag.ContinueOnError)
var items2 []string
fs2.StringArrayVar(&items2, "items", nil, "test items")
err := fs2.Parse([]string{"--items", stringRepr})
if err != nil {
fmt.Printf(" Parse error: %v\n", err)
} else {
fmt.Printf(" Parsed result: %v\n", items2)
fmt.Printf(" Array length: %d\n", len(items2))
if len(items2) == 1 && strings.Contains(items2[0], "[") {
fmt.Printf(" ❌ PROBLEM: Entire bracket string treated as single element!\n")
} else if len(items2) == 3 {
fmt.Printf(" ✓ OK: Array elements preserved correctly\n")
}
}
// Test 3: What about unbracketed comma-separated?
fmt.Println("\n[Test 3] Parsing unbracketed comma-separated string:")
fs3 := pflag.NewFlagSet("test3", pflag.ContinueOnError)
var items3 []string
fs3.StringArrayVar(&items3, "items", nil, "test items")
err = fs3.Parse([]string{"--items", "a,b,c"})
if err != nil {
fmt.Printf(" Parse error: %v\n", err)
} else {
fmt.Printf(" Parsed result: %v\n", items3)
fmt.Printf(" Array length: %d\n", len(items3))
}
}
EOF
cd . && go run /tmp/test_array_parse.goRepository: migtools/oadp-cli
Length of output: 525
🏁 Script executed:
# Let me also verify what the actual test in builder_test.go checks
cat -n cmd/non-admin/verbs/builder_test.go | sed -n '115,125p'Repository: migtools/oadp-cli
Length of output: 437
Fix stringArray round-trip parsing: remove bracket notation before passing to subcommand.
The String() representation of stringArray flags returns bracket-enclosed format ([a,b,c]), but when this is passed directly to the fresh subcommand's parser as --flag [a,b,c], pflag treats the entire bracket string as a single array element, resulting in [[a,b,c]] instead of [a,b,c]. The array elements are lost.
Verification result
[Test 1] Initial parse: [a b c]
[Test 2] String() output: "[a,b,c]"
[Test 3] Re-parse String(): [[a,b,c]] ← entire bracket string becomes 1 element
Either strip the brackets before passing to the parser, or reconstruct using repeated flags (--flag a --flag b --flag c). The existing test only verifies flag.Changed and flag.Value.String() exist, but does not validate that array elements are correctly parsed by the delegated command.
🤖 Prompt for AI Agents
In `@cmd/non-admin/verbs/builder.go` around lines 173 - 181, The
stringArray/stringSlice branch currently appends the entire bracketed String()
output as one value causing a nested array on re-parse; change the logic in the
case "stringArray", "stringSlice" to detect and handle bracket-enclosed values:
if flagValue starts with "[" and ends with "]", trim the brackets, split the
inner string on commas, trim whitespace from each item, and for each non-empty
item append "--"+flag.Name and the item to remainingArgs (so repeated flags are
passed); otherwise keep the existing single-value append behavior for
non-bracket values. Ensure you continue to check flagValue != "" && flagValue !=
"[]".
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/non-admin/verbs/builder_test.go (1)
267-275:⚠️ Potential issue | 🟡 MinorAvoid nil deref in error assertion.
If
erris nil, the test continues and callserr.Error(), which will panic. Prefert.Fatalto stop the test immediately.Suggested fix
err := verbCmd.Execute() if err == nil { - t.Error("Expected error for unknown resource type, got nil") + t.Fatal("Expected error for unknown resource type, got nil") }
🤖 Fix all issues with AI agents
In `@cmd/non-admin/verbs/builder_test.go`:
- Around line 500-507: The test currently only checks flagReceived and
len(receivedMap); update it to assert the exact expected map contents by
defining an expected map (e.g., expectedMap) and comparing it to receivedMap
using a deep-equality check (reflect.DeepEqual or cmp.Diff) and failing the test
with a clear message if they differ; keep flagReceived check but replace the
loose len(receivedMap) branch with an equality assertion that verifies both keys
and values match, and include the mismatched contents in the error message so
failures show the expected vs actual maps (referencing flagReceived and
receivedMap in builder_test.go).
- Around line 453-457: The test currently only logs when the received duration
is not normalized, so change that log to a real assertion: replace the t.Logf in
the builder_test.go check that inspects flagReceived and receivedValue with
t.Fatalf or t.Errorf (e.g., t.Fatalf("Duration flag passed with value %q;
expected normalized form %q", receivedValue, "720h0m0s")) so the test fails on
regressions; keep the existing condition and variables (flagReceived,
receivedValue) and the expected string "720h0m0s".
Comment on lines
453
to
457
| if !flagReceived { | ||
| t.Error("Expected duration flag --ttl to be passed to delegated command") | ||
| } else if receivedValue != "720h0m0s" { | ||
| t.Logf("Duration flag passed with value: %s (expected normalized form)", receivedValue) | ||
| } |
There was a problem hiding this comment.
Make duration normalization a real assertion.
The test logs when the value is unexpected but doesn’t fail, so regressions won’t be caught.
Suggested fix
- if !flagReceived {
- t.Error("Expected duration flag --ttl to be passed to delegated command")
- } else if receivedValue != "720h0m0s" {
- t.Logf("Duration flag passed with value: %s (expected normalized form)", receivedValue)
- }
+ if !flagReceived {
+ t.Fatal("Expected duration flag --ttl to be passed to delegated command")
+ }
+ if receivedValue != "720h0m0s" {
+ t.Errorf("Expected normalized duration value %q, got %q", "720h0m0s", receivedValue)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if !flagReceived { | |
| t.Error("Expected duration flag --ttl to be passed to delegated command") | |
| } else if receivedValue != "720h0m0s" { | |
| t.Logf("Duration flag passed with value: %s (expected normalized form)", receivedValue) | |
| } | |
| if !flagReceived { | |
| t.Fatal("Expected duration flag --ttl to be passed to delegated command") | |
| } | |
| if receivedValue != "720h0m0s" { | |
| t.Errorf("Expected normalized duration value %q, got %q", "720h0m0s", receivedValue) | |
| } |
🤖 Prompt for AI Agents
In `@cmd/non-admin/verbs/builder_test.go` around lines 453 - 457, The test
currently only logs when the received duration is not normalized, so change that
log to a real assertion: replace the t.Logf in the builder_test.go check that
inspects flagReceived and receivedValue with t.Fatalf or t.Errorf (e.g.,
t.Fatalf("Duration flag passed with value %q; expected normalized form %q",
receivedValue, "720h0m0s")) so the test fails on regressions; keep the existing
condition and variables (flagReceived, receivedValue) and the expected string
"720h0m0s".
Comment on lines
500
to
507
| if !flagReceived { | ||
| t.Error("Expected map flag --config to be passed to delegated command") | ||
| } else { | ||
| t.Logf("Map flag passed successfully with %d entries", len(receivedMap)) | ||
| if len(receivedMap) > 0 { | ||
| t.Logf("Map contents: %v", receivedMap) | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert expected map contents, not just presence.
The test currently passes even if only one entry is parsed or values are wrong.
Suggested fix
if !flagReceived {
- t.Error("Expected map flag --config to be passed to delegated command")
- } else {
- t.Logf("Map flag passed successfully with %d entries", len(receivedMap))
- if len(receivedMap) > 0 {
- t.Logf("Map contents: %v", receivedMap)
- }
- }
+ t.Fatal("Expected map flag --config to be passed to delegated command")
+ }
+ if len(receivedMap) != 2 || receivedMap["region"] != "us-east-1" || receivedMap["bucket"] != "my-bucket" {
+ t.Errorf("Expected map entries {region: us-east-1, bucket: my-bucket}, got: %v", receivedMap)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if !flagReceived { | |
| t.Error("Expected map flag --config to be passed to delegated command") | |
| } else { | |
| t.Logf("Map flag passed successfully with %d entries", len(receivedMap)) | |
| if len(receivedMap) > 0 { | |
| t.Logf("Map contents: %v", receivedMap) | |
| } | |
| } | |
| if !flagReceived { | |
| t.Fatal("Expected map flag --config to be passed to delegated command") | |
| } | |
| if len(receivedMap) != 2 || receivedMap["region"] != "us-east-1" || receivedMap["bucket"] != "my-bucket" { | |
| t.Errorf("Expected map entries {region: us-east-1, bucket: my-bucket}, got: %v", receivedMap) | |
| } |
🤖 Prompt for AI Agents
In `@cmd/non-admin/verbs/builder_test.go` around lines 500 - 507, The test
currently only checks flagReceived and len(receivedMap); update it to assert the
exact expected map contents by defining an expected map (e.g., expectedMap) and
comparing it to receivedMap using a deep-equality check (reflect.DeepEqual or
cmp.Diff) and failing the test with a clear message if they differ; keep
flagReceived check but replace the loose len(receivedMap) branch with an
equality assertion that verifies both keys and values match, and include the
mismatched contents in the error message so failures show the expected vs actual
maps (referencing flagReceived and receivedMap in builder_test.go).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/non-admin/verbs/builder.go`:
- Around line 140-153: The current flow in builder.go parses flags and then
directly calls subCmd.RunE, which bypasses any argument validators defined via
subCmd.Args and skips important checks (e.g., backup/delete.go). Before invoking
RunE, call the Args validator by executing subCmd.Args()(or
subCmd.Args()(parsedArgs) depending on signature) with parsedArgs and
handle/return any error; only if that succeeds proceed to call
subCmd.RunE(subCmd, parsedArgs). Ensure you reference the variables and
functions used here: subCmd, remainingArgs, parsedArgs, subCmd.ParseFlags,
subCmd.Flags().Args(), subCmd.Args(), and subCmd.RunE.
🧹 Nitpick comments (1)
cmd/non-admin/verbs/builder.go (1)
65-84: Optional: sort resource types for deterministic error messages.
Map iteration order is random, so sorting avoids noisy diffs and user‑facing variability.♻️ Suggested tweak
import ( "fmt" + "sort" "strings" @@ - var types []string - for resourceType := range vb.resourceRegistry { - types = append(types, resourceType) - } - - var builder strings.Builder - for i, t := range types { - if i > 0 { - builder.WriteString(", ") - } - builder.WriteString(t) - } - return builder.String() + types := make([]string, 0, len(vb.resourceRegistry)) + for resourceType := range vb.resourceRegistry { + types = append(types, resourceType) + } + sort.Strings(types) + return strings.Join(types, ", ")
Joeavaikath
force-pushed
the
bugfixes
branch
from
e336a0e to
913bd65
Compare
Signed-off-by: Joseph <jvaikath@redhat.com>
Contributor
Author
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
Tip For best results, initiate chat on the files or code changes.
I can help you with:
What would you like me to do? |
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
Velero's output.PrintWithFormat uses an upstream scheme that doesn't include NonAdmin CRD types, causing "no kind is registered" errors when using -o yaml or -o json flags. Implemented custom output package with NonAdmin scheme registration to enable proper serialization of downstream types. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Joseph <jvaikath@redhat.com>
Contributor
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Contributor
Author
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
Contributor
|
@stillalearner please have a look |
kaovilai
reviewed
Feb 16, 2026
Comment on lines
+107
to
+138
| // Use the original subcommand directly (not a copy) and set its args | ||
| // This ensures the RunE closure uses the correct flag variables | ||
| subCmd.SetArgs(remainingArgs) | ||
|
|
||
| // Parse flags from the args we set | ||
| // IMPORTANT: We cannot use subCmd.Execute() here because it causes Cobra to | ||
| // re-parse os.Args from the root command, which results in incorrect command | ||
| // path parsing (e.g., "unknown command 'na' for 'backup'" error). | ||
| // Instead, we manually parse flags and execute the RunE function directly. | ||
| if err := subCmd.ParseFlags(remainingArgs); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Validate arguments | ||
| if subCmd.Args != nil { | ||
| // Get the args after flag parsing | ||
| flags := subCmd.Flags() | ||
| argsList := flags.Args() | ||
| if err := subCmd.Args(subCmd, argsList); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| // Execute the RunE function directly with parsed args | ||
| if subCmd.RunE != nil { | ||
| flags := subCmd.Flags() | ||
| return subCmd.RunE(subCmd, flags.Args()) | ||
| } else if subCmd.Run != nil { | ||
| flags := subCmd.Flags() | ||
| subCmd.Run(subCmd, flags.Args()) | ||
| return nil | ||
| } |
Member
There was a problem hiding this comment.
The new delegation bypasses Execute() and directly calls RunE/Run. This means any PreRunE, PersistentPreRunE, PostRunE, or PersistentPostRunE hooks on the subcommand or its parents will not execute. If any of the delegated commands (describe, get, create) rely on these hooks for setup (e.g., initializing clients, validation), they will silently break.
Member
There was a problem hiding this comment.
It would be worth checking whether any delegated commands use PreRunE hooks. If they do, those need to be invoked manually too.
Contributor
Author
There was a problem hiding this comment.
This builder pattern shouldn't have been used since cobra already does this when we use AddCommand
Testing locally that nothing has changed else should be good
kaovilai
reviewed
Feb 16, 2026
Comment on lines
+51
to
+52
|
|
||
| vendor/ No newline at end of file |
Member
There was a problem hiding this comment.
any reason you vendored locally?
Contributor
Author
There was a problem hiding this comment.
inspecting the upstream velero implementations for some funcs that I'm using
eg the output format for -o yaml
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Joseph <jvaikath@redhat.com>
Joeavaikath
force-pushed
the
bugfixes
branch
from
e11877a to
566bb3b
Compare
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@cmd/non-admin/output/output_test.go`:
- Around line 243-257: The os.Pipe() call currently ignores its returned error;
update the test helpers around PrintWithFormat (in TestPrintWithFormat and
TestPrintWithFormatList) to capture the two-value result (r, w, err :=
os.Pipe()), check err immediately, and call t.Fatalf("os.Pipe failed: %v", err)
(or t.Fatal) if non-nil so the test fails fast; keep the rest of the stdout
capture/restore logic the same and ensure you mirror this change in the second
occurrence referenced by TestPrintWithFormatList.
In `@cmd/non-admin/output/output.go`:
- Around line 35-50: NonAdminScheme currently allocates and registers a new
runtime.Scheme on every call (and is invoked frequently via encoderFor); change
it to return a package-level cached singleton by adding a package-scoped
variable (e.g., nonAdminScheme *runtime.Scheme) and a sync.Once (e.g.,
nonAdminSchemeOnce), move the creation and AddToScheme calls inside
nonAdminSchemeOnce.Do(...) in NonAdminScheme, and have NonAdminScheme simply
return the cached nonAdminScheme; ensure the init block is only run once to
preserve thread-safety and avoid re-registering types.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@cmd/non-admin/output/output_test.go`: - Around line 243-257: The os.Pipe() call currently ignores its returned error; update the test helpers around PrintWithFormat (in TestPrintWithFormat and TestPrintWithFormatList) to capture the two-value result (r, w, err := os.Pipe()), check err immediately, and call t.Fatalf("os.Pipe failed: %v", err) (or t.Fatal) if non-nil so the test fails fast; keep the rest of the stdout capture/restore logic the same and ensure you mirror this change in the second occurrence referenced by TestPrintWithFormatList. In `@cmd/non-admin/output/output.go`: - Around line 35-50: NonAdminScheme currently allocates and registers a new runtime.Scheme on every call (and is invoked frequently via encoderFor); change it to return a package-level cached singleton by adding a package-scoped variable (e.g., nonAdminScheme *runtime.Scheme) and a sync.Once (e.g., nonAdminSchemeOnce), move the creation and AddToScheme calls inside nonAdminSchemeOnce.Do(...) in NonAdminScheme, and have NonAdminScheme simply return the cached nonAdminScheme; ensure the init block is only run once to preserve thread-safety and avoid re-registering types.cmd/non-admin/output/output_test.go (1)
243-257: Consider handling theos.Pipe()error.The error from
os.Pipe()is currently ignored. While unlikely to fail in test environments, handling it improves robustness.🧹 Minor fix
// Capture stdout oldStdout := os.Stdout - r, w, _ := os.Pipe() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("Failed to create pipe: %v", err) + } os.Stdout = wNote: The same pattern appears at lines 341-343 in
TestPrintWithFormatList.🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@cmd/non-admin/output/output_test.go` around lines 243 - 257, The os.Pipe() call currently ignores its returned error; update the test helpers around PrintWithFormat (in TestPrintWithFormat and TestPrintWithFormatList) to capture the two-value result (r, w, err := os.Pipe()), check err immediately, and call t.Fatalf("os.Pipe failed: %v", err) (or t.Fatal) if non-nil so the test fails fast; keep the rest of the stdout capture/restore logic the same and ensure you mirror this change in the second occurrence referenced by TestPrintWithFormatList.cmd/non-admin/output/output.go (1)
35-50: Consider caching the scheme as a package-level singleton.
NonAdminScheme()creates a newruntime.Schemeinstance and registers types on every call. Since the scheme is immutable after creation, this function is called repeatedly inencoderFor()(line 127), creating unnecessary allocations during encoding operations.♻️ Suggested refactor using sync.Once
+import "sync" + +var ( + nonAdminScheme *runtime.Scheme + nonAdminSchemeOnce sync.Once +) + // NonAdminScheme returns a runtime.Scheme with NonAdmin types registered func NonAdminScheme() *runtime.Scheme { - scheme := runtime.NewScheme() - - // Add NonAdmin types - if err := nacv1alpha1.AddToScheme(scheme); err != nil { - panic(fmt.Sprintf("failed to add NonAdmin types to scheme: %v", err)) - } - - // Add Velero types for compatibility - if err := velerov1api.AddToScheme(scheme); err != nil { - panic(fmt.Sprintf("failed to add Velero types to scheme: %v", err)) - } - - return scheme + nonAdminSchemeOnce.Do(func() { + nonAdminScheme = runtime.NewScheme() + + // Add NonAdmin types + if err := nacv1alpha1.AddToScheme(nonAdminScheme); err != nil { + panic(fmt.Sprintf("failed to add NonAdmin types to scheme: %v", err)) + } + + // Add Velero types for compatibility + if err := velerov1api.AddToScheme(nonAdminScheme); err != nil { + panic(fmt.Sprintf("failed to add Velero types to scheme: %v", err)) + } + }) + return nonAdminScheme }🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@cmd/non-admin/output/output.go` around lines 35 - 50, NonAdminScheme currently allocates and registers a new runtime.Scheme on every call (and is invoked frequently via encoderFor); change it to return a package-level cached singleton by adding a package-scoped variable (e.g., nonAdminScheme *runtime.Scheme) and a sync.Once (e.g., nonAdminSchemeOnce), move the creation and AddToScheme calls inside nonAdminSchemeOnce.Do(...) in NonAdminScheme, and have NonAdminScheme simply return the cached nonAdminScheme; ensure the init block is only run once to preserve thread-safety and avoid re-registering types.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Signed-off-by: Joseph <jvaikath@redhat.com>
kaovilai
approved these changes
Feb 17, 2026
weshayutin
approved these changes
Feb 17, 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
3 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
How to test the changes made
oc oadp na backup describe <backup-name>should now respect --detailsSummary by CodeRabbit
New Features
Improvements
Tests
Chores