refactor(config): unified Resolve() per spec type for wire→domain conversion#789
Merged
Conversation
…version Wire types in pkg/config (BundleSpec, RecipeSpec, etc.) are intentionally string-typed and forgiving. Conversion to domain types previously happened in three places: pkg/config/validate.go (parse-and-discard), pkg/cli/recipe.go applyCriteriaFromConfig (parse interleaved with override-logging), and pkg/cli/bundle.go parseBundleCmdOptions (string accessors threaded through parsers at consumption time). This commit consolidates the conversion to a single boundary in pkg/config: - (*BundleSpec).Resolve() returns *BundleResolved with typed domain values (DeployerType, *oci.Reference, []ComponentPath, []corev1.Toleration, *corev1.Taint, etc.). Errors carry "spec.bundle.<path>" attribution. - (*RecipeSpec).ResolveCriteria() returns *recipe.Criteria with parsed enums, leaving unset fields zero so callers can detect what to copy. Both methods are nil-receiver tolerant, never return nil pointers, and preserve the nil-vs-explicitly-empty distinction for maps and slices (matching the existing accessor semantics). Validate methods now delegate to Resolve* (no parallel parser to maintain), applyCriteriaFromConfig becomes "Resolve, then merge non-zero fields with override logging," and parseBundleCmdOptions consumes BundleResolved and layers CLI flag overrides on typed values via small helpers (resolveDeployer, resolveTolerations, resolveComponentPaths, resolveTaint, resolveOutputTarget). The redundant per-field bundle accessors are removed since BundleResolved is the single consumption point. Existing recipe accessors (SnapshotPath, OutputPath, OutputFormat, DataDir) stay because they have no enum parsing. Behavior is preserved end-to-end including edge cases like empty CLI strings (--deployer "", --output "", --workload-gate "") that previously masked config and fell through to defaults. Fixes #783
📝 WalkthroughWalkthroughThis PR consolidates wire-to-domain type conversion across three redundant parsing paths into dedicated Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
🤖 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 `@pkg/cli/bundle_resolve_helpers_test.go`:
- Around line 31-440: Group the individual tests for each helper
(resolveDeployer, resolveComponentPaths, resolveTolerations, resolveTaint,
resolveOutputTarget) into single table-driven tests using a slice of test cases
and t.Run subtests; for each family replace the multiple TestResolveXxx_*
functions with one TestResolveXxx_TableDriven that defines []struct{name, args,
want, wantErr, note} and iterates cases calling the same helper
(resolveDeployer, resolveComponentPaths, resolveTolerations, resolveTaint,
resolveOutputTarget) inside t.Run, preserving the original scenarios (flag set,
no flag fallback, empty flag behavior, invalid flag error, override logging) and
reusing runWith and the same assertions per case so behavior is identical but
consolidated into subtests for clarity and reduced duplication.
In `@pkg/config/resolve.go`:
- Around line 170-176: The config path currently copies
b.Scheduling.StorageClass verbatim which allows whitespace-only values; update
the block handling b.Scheduling in resolve.go to trim whitespace from
b.Scheduling.StorageClass (e.g., strings.TrimSpace) and if the trimmed result is
empty return an invalid-request error (same style as the nodes check) instead of
assigning it to out.StorageClass, so config validation matches the CLI semantics
and Validate() will catch blank storageClass values at the spec/resolution step.
- Around line 35-38: The exported BundleResolved contract claims maps and slices
preserve nil-vs-explicitly-empty semantics but Resolve() currently normalizes
toleration slices via snapshotter.ParseTolerations, breaking that guarantee;
either narrow the godoc on BundleResolved to only promise nil-vs-empty for
fields that are actually preserved or change Resolve/BundleResolved handling so
toleration fields are not normalized (stop calling snapshotter.ParseTolerations
or preserve an explicit-empty marker and propagate it through Resolve), updating
BundleResolved godoc and the Resolve() implementation and referencing the
toleration slice fields, the BundleResolved type, the Resolve function, and
snapshotter.ParseTolerations to keep comments and behavior consistent.
- Around line 238-276: Replace the errors.Wrap calls in ResolveCriteria with
errors.PropagateOrWrap so you preserve structured error codes returned from the
recipe parse functions; specifically, where you call
recipe.ParseCriteriaServiceType, recipe.ParseCriteriaAcceleratorType,
recipe.ParseCriteriaIntentType, recipe.ParseCriteriaOSType, and
recipe.ParseCriteriaPlatformType, change the error handling from
errors.Wrap(errors.ErrCodeInvalidRequest, "...") to errors.PropagateOrWrap(err,
errors.ErrCodeInvalidRequest, "invalid spec.recipe.criteria.<field>") so the
original err from the ParseCriteria* functions is propagated while attaching the
spec-path context.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f2a3e86f-cf93-4372-b5e6-8bdb068d75ea
📒 Files selected for processing (9)
pkg/cli/bundle.gopkg/cli/bundle_config.gopkg/cli/bundle_resolve_helpers_test.gopkg/cli/recipe.gopkg/config/accessors.gopkg/config/accessors_test.gopkg/config/resolve.gopkg/config/resolve_test.gopkg/config/validate.go
Comment on lines
+31
to
+440
| func TestResolveDeployer_FlagSetWinsOverConfig(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "deployer"}} | ||
| runWith(t, flags, []string{"--deployer", "argocd"}, func(c *cli.Command) { | ||
| got, err := resolveDeployer(c, bundlercfg.DeployerHelm) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got != bundlercfg.DeployerArgoCD { | ||
| t.Errorf("got %q, want argocd", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveDeployer_NoFlagUsesConfigFallback(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "deployer"}} | ||
| runWith(t, flags, []string{}, func(c *cli.Command) { | ||
| got, err := resolveDeployer(c, bundlercfg.DeployerArgoCD) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got != bundlercfg.DeployerArgoCD { | ||
| t.Errorf("got %q, want argocd from config", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveDeployer_NoFlagNoConfigDefaultsToHelm(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "deployer"}} | ||
| runWith(t, flags, []string{}, func(c *cli.Command) { | ||
| got, err := resolveDeployer(c, bundlercfg.DeployerType("")) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got != bundlercfg.DeployerHelm { | ||
| t.Errorf("got %q, want helm default", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveDeployer_InvalidFlagReturnsError(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "deployer"}} | ||
| runWith(t, flags, []string{"--deployer", "fluxcd"}, func(c *cli.Command) { | ||
| _, err := resolveDeployer(c, bundlercfg.DeployerType("")) | ||
| if err == nil { | ||
| t.Fatal("expected error for invalid deployer") | ||
| } | ||
| if !strings.Contains(err.Error(), "invalid --deployer") { | ||
| t.Errorf("error %q must mention --deployer", err.Error()) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveDeployer_FlagSetEmptyDefaultsToHelm(t *testing.T) { | ||
| // `--deployer ""` matches pre-refactor behavior: empty CLI string | ||
| // masks config and falls through to the Helm default rather than | ||
| // being passed to ParseDeployerType (which rejects ""). | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "deployer"}} | ||
| runWith(t, flags, []string{"--deployer", ""}, func(c *cli.Command) { | ||
| got, err := resolveDeployer(c, bundlercfg.DeployerArgoCD) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got != bundlercfg.DeployerHelm { | ||
| t.Errorf("got %q, want helm default", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| // === resolveComponentPaths === | ||
|
|
||
| func TestResolveComponentPaths_FlagSetParsesCLI(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringSliceFlag{Name: "set"}} | ||
| runWith(t, flags, []string{"--set", "gpuoperator:driver.version=570.0.0"}, | ||
| func(c *cli.Command) { | ||
| got, err := resolveComponentPaths(c, "set", nil, bundlercfg.ParseValueOverrides) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(got) != 1 { | ||
| t.Errorf("got %d entries, want 1", len(got)) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveComponentPaths_NoFlagReturnsFallback(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringSliceFlag{Name: "set"}} | ||
| fallback := []bundlercfg.ComponentPath{{Component: "x"}} | ||
| runWith(t, flags, []string{}, func(c *cli.Command) { | ||
| got, err := resolveComponentPaths(c, "set", fallback, bundlercfg.ParseValueOverrides) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(got) != 1 || got[0].Component != "x" { | ||
| t.Errorf("expected fallback returned, got %v", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveComponentPaths_NoFlagNilFallbackReturnsNil(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringSliceFlag{Name: "set"}} | ||
| runWith(t, flags, []string{}, func(c *cli.Command) { | ||
| got, err := resolveComponentPaths(c, "set", nil, bundlercfg.ParseValueOverrides) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got != nil { | ||
| t.Errorf("expected nil, got %v", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveComponentPaths_InvalidFlagReturnsError(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringSliceFlag{Name: "set"}} | ||
| runWith(t, flags, []string{"--set", "no-equals-sign"}, func(c *cli.Command) { | ||
| _, err := resolveComponentPaths(c, "set", nil, bundlercfg.ParseValueOverrides) | ||
| if err == nil { | ||
| t.Fatal("expected error") | ||
| } | ||
| if !strings.Contains(err.Error(), "invalid --set") { | ||
| t.Errorf("error %q must mention --set", err.Error()) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveComponentPaths_FlagOverridesNonEmptyConfig(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringSliceFlag{Name: "set"}} | ||
| fallback := []bundlercfg.ComponentPath{{Component: "old"}} | ||
| runWith(t, flags, []string{"--set", "gpuoperator:driver.version=570.0.0"}, | ||
| func(c *cli.Command) { | ||
| got, err := resolveComponentPaths(c, "set", fallback, bundlercfg.ParseValueOverrides) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(got) != 1 || got[0].Component == "old" { | ||
| t.Errorf("expected CLI to replace fallback, got %v", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| // === resolveTolerations === | ||
|
|
||
| func TestResolveTolerations_FlagSetParsesCLI(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringSliceFlag{Name: "tol"}} | ||
| runWith(t, flags, []string{"--tol", "k=v:NoSchedule"}, func(c *cli.Command) { | ||
| got, err := resolveTolerations(c, "tol", nil) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(got) != 1 || got[0].Key != "k" { | ||
| t.Errorf("got %v", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveTolerations_NoFlagNilFallbackUsesDefault(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringSliceFlag{Name: "tol"}} | ||
| runWith(t, flags, []string{}, func(c *cli.Command) { | ||
| got, err := resolveTolerations(c, "tol", nil) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| // DefaultTolerations is a single Toleration{Op: Exists}. | ||
| if len(got) != 1 || got[0].Operator != corev1.TolerationOpExists { | ||
| t.Errorf("expected DefaultTolerations, got %v", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveTolerations_NoFlagNonNilFallbackReturnsFallback(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringSliceFlag{Name: "tol"}} | ||
| fallback := []corev1.Toleration{{Key: "from-config"}} | ||
| runWith(t, flags, []string{}, func(c *cli.Command) { | ||
| got, err := resolveTolerations(c, "tol", fallback) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if len(got) != 1 || got[0].Key != "from-config" { | ||
| t.Errorf("expected fallback, got %v", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveTolerations_NoFlagEmptyNonNilFallbackReturnsFallback(t *testing.T) { | ||
| // Explicitly empty (non-nil) fallback round-trips as an empty | ||
| // (non-nil) slice — the parser is not invoked when the flag is unset. | ||
| flags := []cli.Flag{&cli.StringSliceFlag{Name: "tol"}} | ||
| fallback := []corev1.Toleration{} | ||
| runWith(t, flags, []string{}, func(c *cli.Command) { | ||
| got, err := resolveTolerations(c, "tol", fallback) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got == nil { | ||
| t.Fatal("expected non-nil empty, got nil") | ||
| } | ||
| if len(got) != 0 { | ||
| t.Errorf("expected empty, got %v", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveTolerations_InvalidFlagReturnsError(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringSliceFlag{Name: "tol"}} | ||
| runWith(t, flags, []string{"--tol", "malformed"}, func(c *cli.Command) { | ||
| _, err := resolveTolerations(c, "tol", nil) | ||
| if err == nil { | ||
| t.Fatal("expected error") | ||
| } | ||
| if !strings.Contains(err.Error(), "invalid --tol") { | ||
| t.Errorf("error %q must mention --tol", err.Error()) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| // === resolveTaint === | ||
|
|
||
| func TestResolveTaint_FlagSetParsesCLI(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "gate"}} | ||
| runWith(t, flags, []string{"--gate", "k=v:NoSchedule"}, func(c *cli.Command) { | ||
| got, err := resolveTaint(c, "gate", nil) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got == nil || got.Key != "k" { | ||
| t.Errorf("got %+v", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveTaint_FlagSetEmptyMasksFallback(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "gate"}} | ||
| fallback := &corev1.Taint{Key: "from-config"} | ||
| // Explicit empty CLI value masks the config fallback and yields nil. | ||
| // This preserves pre-refactor behavior where stringFlagOrConfig | ||
| // surfaced "" and the caller skipped parsing entirely. | ||
| runWith(t, flags, []string{"--gate", ""}, func(c *cli.Command) { | ||
| got, err := resolveTaint(c, "gate", fallback) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got != nil { | ||
| t.Errorf("expected nil (CLI empty masks fallback), got %+v", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveTaint_NoFlagReturnsFallback(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "gate"}} | ||
| fallback := &corev1.Taint{Key: "from-config"} | ||
| runWith(t, flags, []string{}, func(c *cli.Command) { | ||
| got, err := resolveTaint(c, "gate", fallback) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got == nil || got.Key != "from-config" { | ||
| t.Errorf("expected fallback, got %+v", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveTaint_NoFlagNilFallbackReturnsNil(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "gate"}} | ||
| runWith(t, flags, []string{}, func(c *cli.Command) { | ||
| got, err := resolveTaint(c, "gate", nil) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got != nil { | ||
| t.Errorf("expected nil, got %+v", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveTaint_FlagOverridesFallbackLogsOverride(t *testing.T) { | ||
| // Just exercise the override branch; we don't assert the log. | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "gate"}} | ||
| fallback := &corev1.Taint{Key: "old"} | ||
| runWith(t, flags, []string{"--gate", "new=v:NoSchedule"}, func(c *cli.Command) { | ||
| got, err := resolveTaint(c, "gate", fallback) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if got == nil || got.Key != "new" { | ||
| t.Errorf("expected new taint, got %+v", got) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveTaint_InvalidFlagReturnsError(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "gate"}} | ||
| runWith(t, flags, []string{"--gate", "no-effect"}, func(c *cli.Command) { | ||
| _, err := resolveTaint(c, "gate", nil) | ||
| if err == nil { | ||
| t.Fatal("expected error") | ||
| } | ||
| if !strings.Contains(err.Error(), "invalid --gate") { | ||
| t.Errorf("error %q must mention --gate", err.Error()) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| // === resolveOutputTarget === | ||
|
|
||
| func TestResolveOutputTarget_FlagSetParsesCLI(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "output"}} | ||
| resolved := &appcfg.BundleResolved{} | ||
| runWith(t, flags, []string{"--output", "./mybundle"}, func(c *cli.Command) { | ||
| ref, err := resolveOutputTarget(c, resolved) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ref == nil || ref.IsOCI { | ||
| t.Errorf("expected local ref, got %+v", ref) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveOutputTarget_NoFlagUsesResolvedTarget(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "output"}} | ||
| b := &appcfg.BundleSpec{Output: &appcfg.BundleOutputSpec{Target: "./from-config"}} | ||
| resolved, err := b.Resolve() | ||
| if err != nil { | ||
| t.Fatalf("Resolve: %v", err) | ||
| } | ||
| runWith(t, flags, []string{}, func(c *cli.Command) { | ||
| ref, err := resolveOutputTarget(c, resolved) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ref == nil { | ||
| t.Fatal("expected ref, got nil") | ||
| } | ||
| if ref.LocalPath == "" { | ||
| t.Errorf("expected LocalPath populated, got %+v", ref) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveOutputTarget_NoFlagNoResolvedDefaultsToCurrentDir(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "output"}} | ||
| resolved := &appcfg.BundleResolved{} | ||
| runWith(t, flags, []string{}, func(c *cli.Command) { | ||
| ref, err := resolveOutputTarget(c, resolved) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ref == nil || ref.IsOCI { | ||
| t.Errorf("expected local ref defaulting to '.', got %+v", ref) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveOutputTarget_InvalidFlagReturnsError(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "output"}} | ||
| resolved := &appcfg.BundleResolved{} | ||
| runWith(t, flags, []string{"--output", "oci://"}, func(c *cli.Command) { | ||
| _, err := resolveOutputTarget(c, resolved) | ||
| if err == nil { | ||
| t.Fatal("expected error") | ||
| } | ||
| if !strings.Contains(err.Error(), "invalid --output") { | ||
| t.Errorf("error %q must mention --output", err.Error()) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveOutputTarget_FlagSetEmptyDefaultsToCurrentDir(t *testing.T) { | ||
| // `--output ""` matches pre-refactor behavior where the empty | ||
| // string was substituted with "." before parsing rather than | ||
| // passed through (which would have produced LocalPath: ""). | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "output"}} | ||
| b := &appcfg.BundleSpec{Output: &appcfg.BundleOutputSpec{Target: "./from-config"}} | ||
| resolved, err := b.Resolve() | ||
| if err != nil { | ||
| t.Fatalf("Resolve: %v", err) | ||
| } | ||
| runWith(t, flags, []string{"--output", ""}, func(c *cli.Command) { | ||
| ref, err := resolveOutputTarget(c, resolved) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ref == nil || ref.IsOCI { | ||
| t.Errorf("expected local ref defaulting to '.', got %+v", ref) | ||
| } | ||
| if ref.LocalPath != "." { | ||
| t.Errorf("expected LocalPath '.', got %q", ref.LocalPath) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| func TestResolveOutputTarget_FlagOverridesNonEmptyConfigLogs(t *testing.T) { | ||
| flags := []cli.Flag{&cli.StringFlag{Name: "output"}} | ||
| b := &appcfg.BundleSpec{Output: &appcfg.BundleOutputSpec{Target: "./old"}} | ||
| resolved, err := b.Resolve() | ||
| if err != nil { | ||
| t.Fatalf("Resolve: %v", err) | ||
| } | ||
| runWith(t, flags, []string{"--output", "./new"}, func(c *cli.Command) { | ||
| ref, err := resolveOutputTarget(c, resolved) | ||
| if err != nil { | ||
| t.Fatalf("unexpected error: %v", err) | ||
| } | ||
| if ref == nil { | ||
| t.Fatal("expected ref") | ||
| } | ||
| if !strings.HasSuffix(ref.LocalPath, "new") { | ||
| t.Errorf("expected ./new override, got %+v", ref) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether this test file uses table-driven patterns today.
fd -i 'bundle_resolve_helpers_test.go' --exec rg -n '^\s*func\s+Test|t\.Run\(|\[\]struct\s*\{' {}Repository: NVIDIA/aicr
Length of output: 2028
Convert test functions into table-driven subtests for each helper family.
This file contains 27 individual test functions organized by helper (resolveDeployer, resolveComponentPaths, resolveTolerations, resolveTaint, resolveOutputTarget) with no table-driven patterns (t.Run subtests or []struct test cases). Group each helper's 5–6 scenario tests into a single table-driven test with subtests to align with Go test conventions and reduce duplication.
🤖 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 `@pkg/cli/bundle_resolve_helpers_test.go` around lines 31 - 440, Group the
individual tests for each helper (resolveDeployer, resolveComponentPaths,
resolveTolerations, resolveTaint, resolveOutputTarget) into single table-driven
tests using a slice of test cases and t.Run subtests; for each family replace
the multiple TestResolveXxx_* functions with one TestResolveXxx_TableDriven that
defines []struct{name, args, want, wantErr, note} and iterates cases calling the
same helper (resolveDeployer, resolveComponentPaths, resolveTolerations,
resolveTaint, resolveOutputTarget) inside t.Run, preserving the original
scenarios (flag set, no flag fallback, empty flag behavior, invalid flag error,
override logging) and reusing runWith and the same assertions per case so
behavior is identical but consolidated into subtests for clarity and reduced
duplication.
Comment on lines
+35
to
+38
| // Zero values mean "config did not set this field." Maps and slices | ||
| // preserve the nil-vs-explicitly-empty distinction from the wire spec — | ||
| // callers can therefore detect whether a user wrote `selector: {}` to | ||
| // clear an inherited default vs. omitted the key entirely. |
There was a problem hiding this comment.
Narrow the exported BundleResolved contract.
This comment says maps and slices preserve nil-vs-explicitly-empty semantics, but Resolve() does not keep that guarantee for toleration slices because explicitly empty toleration lists are normalized by snapshotter.ParseTolerations(...). Please either scope the doc to the fields that actually preserve the distinction or change the implementation so callers do not rely on a contract the API does not provide.
As per coding guidelines, "update code comments and godoc for API changes in Go files."
🤖 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 `@pkg/config/resolve.go` around lines 35 - 38, The exported BundleResolved
contract claims maps and slices preserve nil-vs-explicitly-empty semantics but
Resolve() currently normalizes toleration slices via
snapshotter.ParseTolerations, breaking that guarantee; either narrow the godoc
on BundleResolved to only promise nil-vs-empty for fields that are actually
preserved or change Resolve/BundleResolved handling so toleration fields are not
normalized (stop calling snapshotter.ParseTolerations or preserve an
explicit-empty marker and propagate it through Resolve), updating BundleResolved
godoc and the Resolve() implementation and referencing the toleration slice
fields, the BundleResolved type, the Resolve function, and
snapshotter.ParseTolerations to keep comments and behavior consistent.
Comment on lines
+170
to
+176
| if b.Scheduling != nil { | ||
| if b.Scheduling.Nodes < 0 { | ||
| return nil, errors.New(errors.ErrCodeInvalidRequest, | ||
| fmt.Sprintf("spec.bundle.scheduling.nodes must be >= 0, got %d", b.Scheduling.Nodes)) | ||
| } | ||
| out.Nodes = b.Scheduling.Nodes | ||
| out.StorageClass = b.Scheduling.StorageClass |
There was a problem hiding this comment.
Reject blank spec.bundle.scheduling.storageClass in config too.
The CLI path trims and rejects whitespace-only --storage-class, but the config path copies b.Scheduling.StorageClass verbatim. A quoted YAML value like " " now passes Validate() and later gets injected as the storage class. Trimming and validating here would keep config and CLI semantics aligned and attribute the error to the spec path.
🤖 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 `@pkg/config/resolve.go` around lines 170 - 176, The config path currently
copies b.Scheduling.StorageClass verbatim which allows whitespace-only values;
update the block handling b.Scheduling in resolve.go to trim whitespace from
b.Scheduling.StorageClass (e.g., strings.TrimSpace) and if the trimmed result is
empty return an invalid-request error (same style as the nodes check) instead of
assigning it to out.StorageClass, so config validation matches the CLI semantics
and Validate() will catch blank storageClass values at the spec/resolution step.
Comment on lines
+238
to
+276
| if c.Service != "" { | ||
| v, err := recipe.ParseCriteriaServiceType(c.Service) | ||
| if err != nil { | ||
| return nil, errors.Wrap(errors.ErrCodeInvalidRequest, | ||
| "invalid spec.recipe.criteria.service", err) | ||
| } | ||
| out.Service = v | ||
| } | ||
| if c.Accelerator != "" { | ||
| v, err := recipe.ParseCriteriaAcceleratorType(c.Accelerator) | ||
| if err != nil { | ||
| return nil, errors.Wrap(errors.ErrCodeInvalidRequest, | ||
| "invalid spec.recipe.criteria.accelerator", err) | ||
| } | ||
| out.Accelerator = v | ||
| } | ||
| if c.Intent != "" { | ||
| v, err := recipe.ParseCriteriaIntentType(c.Intent) | ||
| if err != nil { | ||
| return nil, errors.Wrap(errors.ErrCodeInvalidRequest, | ||
| "invalid spec.recipe.criteria.intent", err) | ||
| } | ||
| out.Intent = v | ||
| } | ||
| if c.OS != "" { | ||
| v, err := recipe.ParseCriteriaOSType(c.OS) | ||
| if err != nil { | ||
| return nil, errors.Wrap(errors.ErrCodeInvalidRequest, | ||
| "invalid spec.recipe.criteria.os", err) | ||
| } | ||
| out.OS = v | ||
| } | ||
| if c.Platform != "" { | ||
| v, err := recipe.ParseCriteriaPlatformType(c.Platform) | ||
| if err != nil { | ||
| return nil, errors.Wrap(errors.ErrCodeInvalidRequest, | ||
| "invalid spec.recipe.criteria.platform", err) | ||
| } | ||
| out.Platform = v |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pkg/errors helper =="
fd -p 'errors.go' pkg/errors -x sed -n '/func PropagateOrWrap/,/}/p' {}
echo
echo "== recipe criteria parsers and their error helpers =="
fd -p 'criteria.go' pkg/recipe -x rg -n -C2 'func ParseCriteria(ServiceType|AcceleratorType|IntentType|OSType|PlatformType)|errors\.(New|Wrap|WrapWithContext|PropagateOrWrap)' {}
echo
echo "== current ResolveCriteria wrapping sites =="
rg -n -C1 'invalid spec\.recipe\.criteria\.(service|accelerator|intent|os|platform)' pkg/config/resolve.goRepository: NVIDIA/aicr
Length of output: 12285
Replace errors.Wrap() with errors.PropagateOrWrap() at the pkg/recipe parse boundary.
ResolveCriteria() is the single wire→domain boundary for these fields. Each recipe.ParseCriteria* function already returns a structured error with ErrCodeInvalidRequest, but the current errors.Wrap(errors.ErrCodeInvalidRequest, ...) calls overwrite it. Use errors.PropagateOrWrap(err, errors.ErrCodeInvalidRequest, "message") to preserve the original error code while attaching the spec-path context.
Lines 238–276 (all five criteria fields)
if c.Service != "" {
v, err := recipe.ParseCriteriaServiceType(c.Service)
if err != nil {
- return nil, errors.Wrap(errors.ErrCodeInvalidRequest,
- "invalid spec.recipe.criteria.service", err)
+ return nil, errors.PropagateOrWrap(err, errors.ErrCodeInvalidRequest,
+ "invalid spec.recipe.criteria.service")
}
out.Service = v
}Apply the same pattern to Accelerator, Intent, OS, and Platform fields.
🤖 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 `@pkg/config/resolve.go` around lines 238 - 276, Replace the errors.Wrap calls
in ResolveCriteria with errors.PropagateOrWrap so you preserve structured error
codes returned from the recipe parse functions; specifically, where you call
recipe.ParseCriteriaServiceType, recipe.ParseCriteriaAcceleratorType,
recipe.ParseCriteriaIntentType, recipe.ParseCriteriaOSType, and
recipe.ParseCriteriaPlatformType, change the error handling from
errors.Wrap(errors.ErrCodeInvalidRequest, "...") to errors.PropagateOrWrap(err,
errors.ErrCodeInvalidRequest, "invalid spec.recipe.criteria.<field>") so the
original err from the ParseCriteria* functions is propagated while attaching the
spec-path context.
Contributor
Coverage Report ✅
Coverage BadgeMerging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. |
lalitadithya
approved these changes
May 7, 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.
Summary
Consolidates string→typed conversion of wire-spec fields into a single boundary:
(*BundleSpec).Resolve()and(*RecipeSpec).ResolveCriteria(). Removes the three-way parallel parsing acrossvalidate.go,applyCriteriaFromConfig, andparseBundleCmdOptions.Motivation / Context
Follow-up to #782 (review feedback from @lockwobr). Wire types in
pkg/configare intentionally string-typed; conversion to domain types was happening in three places with subtly different code paths and error attribution. This refactor makes the conversion happen once at the type boundary.Fixes: #783
Related: #782
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)pkg/configImplementation Notes
BundleResolvedis the typed projection ofBundleSpec.(*BundleSpec).Resolve()performs all string→domain conversion (bundlercfg.DeployerType,*oci.Reference,[]bundlercfg.ComponentPath,[]corev1.Toleration,*corev1.Taint, etc.) and attributes parse errors to theirspec.bundle.<path>source.(*RecipeSpec).ResolveCriteria()returns*recipe.Criteriawith parsed enums; unset fields stay zero so callers can detect what to copy onto a target.maps.Clone/ conditional parsing.validate()delegates toResolve*()and discards the result — there's no parallel parser to keep in sync.applyCriteriaFromConfigsimplifies to "call ResolveCriteria, copy non-zero fields, log overrides."parseBundleCmdOptionsconsumes*BundleResolveddirectly. New small CLI helpers inpkg/cli/bundle_config.go(resolveDeployer,resolveTolerations,resolveComponentPaths,resolveTaint) andbundle.go::resolveOutputTargetlayer CLI flag overrides onto typed values.BundleSpecare removed sinceBundleResolvedis now the single consumption point. Recipe accessors stay because their fields have no enum parsing.Behavior preservation for empty CLI strings:
--deployer "",--workload-gate "", and--output ""previously masked config and fell through to documented defaults (Helm / nil / "."). The new helpers preserve this — verified with dedicated tests.Testing
Coverage on changed packages (vs
origin/main):pkg/configpkg/cliNew exported methods at 100% coverage. New CLI helpers at 90–100%. New tests:
pkg/config/resolve_test.go— table-driven, including nil-receiver, all-empty, all-populated, every invalid-enum case, nil-vs-explicitly-empty for selectors / tolerations / value-overrides, defensive map cloning.pkg/config/accessors_test.go— nil-tolerance and population for the remaining recipe-side accessors.pkg/cli/bundle_resolve_helpers_test.go— every branch of every new helper, including the empty-CLI behavior-preservation cases (TestResolveDeployer_FlagSetEmptyDefaultsToHelm,TestResolveTaint_FlagSetEmptyMasksFallback,TestResolveOutputTarget_FlagSetEmptyDefaultsToCurrentDir).All existing integration/E2E tests in
pkg/cli/config_*_test.gocontinue to pass unchanged.Risk Assessment
Rollout notes: No user-visible behavior change; wire format unchanged. One internal API change:
*BundleSpecno longer carries per-field accessors (RecipeInput,OutputTarget,DeploymentDeployer, etc.); callers should use(*BundleSpec).Resolve()instead. No external consumers of those accessors found in the codebase.Checklist
make testwith-race)make lint)git commit -S)