feat(bundler/api): add --dynamic query parameter to bundle handler by lockwobr · Pull Request #603 · NVIDIA/aicr

lockwobr · 2026-04-17T01:05:45Z

Summary

Add dynamic query parameter to POST /v1/bundle for CLI/API parity with the --dynamic flag. Along the way, extract a shared ComponentPath primitive so --set and --dynamic share one parser, and fix two adjacent OpenAPI / godoc doc gaps.

Motivation / Context

PR #527 added --dynamic support to aicr bundle via the CLI (pkg/cli/bundle.go), but the HTTP API handler (pkg/bundler/handler.go) was never updated. Users calling POST /v1/bundle could not declare dynamic install-time values — only CLI users could. This closes that parity gap.

Closes: #572
Related: #515, #527

Type of Change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update
Refactoring (no functional changes)
Build/CI/tooling

Component(s) Affected

CLI (cmd/aicr, pkg/cli)
API server (cmd/aicrd, pkg/api, pkg/server)
Recipe engine / data (pkg/recipe)
Bundlers (pkg/bundler, pkg/component/*)
Collectors / snapshotter (pkg/collector, pkg/snapshotter)
Validator (pkg/validator)
Core libraries (pkg/errors, pkg/k8s)
Docs/examples (docs/, examples/)
Other: ____________

Implementation Notes

Shared ComponentPath primitive (pkg/bundler/config/component_path.go).
Exports ComponentPath{Component, Path string, Value *string}, a Parse method (single tokenizer for component:path[=value]), and a HasValue() helper. ParseValueOverrides
and ParseDynamicValues now return []ComponentPath. New WithValueOverridePaths / WithDynamicValuePaths options consume the slice; the existing map-shaped
WithValueOverrides / WithDynamicValues options stay for direct map-shape test callers.

Handler wiring. Added dynamicValues to bundleParams, parse query["dynamic"] next to set, wire through config.WithDynamicValuePaths, and log a dynamic_declarations
count. Swept the HandleBundles godoc to list all 11 query params the handler actually parses — deployer and repo had been undocumented before this PR.

OpenAPI cleanup. Added dynamic parameter on /v1/bundle (modeled on set). Expanded the deployer enum from [helm, argocd] to [helm, argocd, argocd-helm] —
argocd-helm has been a valid DeployerType since #527 (pkg/bundler/config/config.go:40-43) but was missing from the contract.

Behavior narrowing on --set. safePathPattern (anchored [a-zA-Z0-9][a-zA-Z0-9._-]*) was previously only applied to --dynamic inputs. It now applies to --set as well.
Inputs like bundler:path.{{inject}}=x or bundler:foo/bar=x now return a clear error instead of silently producing literal keys in the Helm values map. No test fixture or
documented Helm values convention relies on characters outside that pattern.

Deliberate non-goals (filed as follow-ups):

dynamic for a registered component that isn't in recipe.componentRefs is silently dropped today by both CLI and API (same behavior, not changed).
parseQueryParams continues to wrap per-parser errors with ErrCodeInvalidRequest even though the inner errors already carry that code — consistent with every other param
parse here. A separate follow-up will eliminate the double-wrap across all params.
Collapsing Config.valueOverrides + Config.dynamicValues into a single per-component struct is a larger refactor deferred to its own PR.

Testing

# Commands run (prefer `make qualify` for non-trivial changes)
make qualify

Risk Assessment

Low — Isolated change, well-tested, easy to revert
Medium — Touches multiple components or has broader impact
High — Breaking change, affects critical paths, or complex rollout

Rollout notes:

Checklist

Tests pass locally (make test with -race)
Linter passes (make lint)
I did not skip/disable tests to make CI green
I added/updated tests for new functionality
I updated docs if user-facing behavior changed
Changes follow existing patterns in the codebase
[ x] Commits are cryptographically signed (git commit -S) — GPG signing info

coderabbitai · 2026-04-17T01:05:53Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: bd575c13-ec01-4178-b66a-ff8bd494a53a

📥 Commits

Reviewing files that changed from the base of the PR and between 17d42f9 and 93f77c5.

⛔ Files ignored due to path filters (10)

vendor/golang.org/x/text/cases/cases.go is excluded by !vendor/**
vendor/golang.org/x/text/cases/context.go is excluded by !vendor/**
vendor/golang.org/x/text/cases/fold.go is excluded by !vendor/**
vendor/golang.org/x/text/cases/icu.go is excluded by !vendor/**
vendor/golang.org/x/text/cases/info.go is excluded by !vendor/**
vendor/golang.org/x/text/cases/map.go is excluded by !vendor/**
vendor/golang.org/x/text/cases/tables15.0.0.go is excluded by !vendor/**
vendor/golang.org/x/text/cases/tables17.0.0.go is excluded by !vendor/**
vendor/golang.org/x/text/cases/trieval.go is excluded by !vendor/**
vendor/modules.txt is excluded by !vendor/**

📒 Files selected for processing (15)

api/aicr/v1/server.yaml
go.mod
pkg/bundler/bundler.go
pkg/bundler/config/component_path.go
pkg/bundler/config/component_path_test.go
pkg/bundler/config/config.go
pkg/bundler/config/config_test.go
pkg/bundler/deployer/argocdhelm/argocdhelm.go
pkg/bundler/deployer/argocdhelm/argocdhelm_test.go
pkg/bundler/handler.go
pkg/bundler/handler_test.go
pkg/cli/bundle.go
pkg/cli/bundle_test.go
pkg/component/overrides.go
pkg/component/overrides_test.go

📝 Walkthrough

Walkthrough

The changes introduce a --dynamic query parameter to the bundle API handler for feature parity with the CLI, backed by a new ComponentPath struct for parsing and validating component value specifications. The configuration parsing layer is refactored to use this abstraction, delegating validation to ComponentPath.Parse while updating return types from nested maps to slices. Reflection-based struct override functionality is removed from the component system, limiting overrides to map-based dot-notation handling via a consolidated navigateParent helper. The ArgoCD Helm deployer switches from post-processing marshaled YAML to using *yaml.Node with block scalar style for proper Helm template rendering. The OpenAPI spec is updated to document the new dynamic parameter and extend the deployer enum to include argocd-helm.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Detailed Notes

This review requires careful attention to several interconnected subsystems:

New abstraction & validation: The ComponentPath struct introduces strict parsing with regex-based path segment validation (^[a-zA-Z0-9][a-zA-Z0-9._-]*$). This replaces distributed validation logic; verify the pattern covers all intended use cases and the error messages are clear.

Signature changes: ParseValueOverrides and ParseDynamicValues now return []ComponentPath instead of nested maps. Audit all call sites to confirm the slice-based approach integrates correctly, especially the differentiation between entries with/without values.

Large removal: The deletion of ~470 lines from pkg/component/overrides.go removes reflection-based struct overrides. Confirm this functionality is no longer needed; check for any orphaned tests or documentation referencing ApplyValueOverrides.

YAML serialization refactoring: The shift from post-processing marshaled bytes to using *yaml.Node with LiteralStyle in the argocd-helm deployer is a structural change affecting helm render-time evaluation. Validate the block scalar formatting produces correct Helm template output.

Test comprehensiveness: Several tests were consolidated or removed (TestParseSetFlags, extensive struct override tests). Ensure coverage remains adequate for the new code paths, particularly error cases in ComponentPath.Parse.

API contract: The handler now accepts dynamic as a repeated query parameter; verify query parameter parsing, error handling, and logging align with existing patterns for similar parameters like set.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: adding a --dynamic query parameter to the bundle handler for feature parity with the CLI.
Description check	✅ Passed	The PR description is comprehensive and clearly related to the changeset, explaining the motivation, implementation details, and rationale for all modifications.
Linked Issues check	✅ Passed	The PR successfully implements all coding requirements from issue `#572`: parsing the dynamic query parameter via config.ParseDynamicValues(), wiring through config.WithDynamicValuePaths(), adding test coverage, and achieving CLI/API parity for dynamic declarations.
Out of Scope Changes check	✅ Passed	All code changes are directly related to the PR objectives. The component_path refactoring, config modifications, handler wiring, and override behavior changes are all necessary to implement dynamic parameter support and improve code quality through shared parsing logic.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/api-dynamic

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

github-actions · 2026-04-17T01:15:19Z

Coverage Report ✅

Metric	Value
Coverage	74.5%
Threshold	70%
Status	Pass

Coverage Badge

![Coverage](https://img.shields.io/badge/coverage-74.5%25-green)

Merging this branch changes the coverage (2 decrease, 2 increase)

Impacted Packages	Coverage Δ	🤖
github.com/NVIDIA/aicr/pkg/bundler	63.22% (+0.22%)	👍
github.com/NVIDIA/aicr/pkg/bundler/config	96.83% (+0.29%)	👍
github.com/NVIDIA/aicr/pkg/bundler/deployer/argocdhelm	69.58% (-2.06%)	👎
github.com/NVIDIA/aicr/pkg/cli	37.70% (ø)
github.com/NVIDIA/aicr/pkg/component	71.02% (-6.63%)	👎

Coverage by file

Changed files (no unit tests)

Changed File	Coverage Δ	Total	Covered	Missed	🤖
github.com/NVIDIA/aicr/pkg/bundler/bundler.go	59.79% (ø)	378	226	152
github.com/NVIDIA/aicr/pkg/bundler/config/component_path.go	100.00% (+100.00%)	30 (+30)	30 (+30)	0	🌟
github.com/NVIDIA/aicr/pkg/bundler/config/config.go	96.23% (-0.31%)	159 (-14)	153 (-14)	6	👎
github.com/NVIDIA/aicr/pkg/bundler/deployer/argocdhelm/argocdhelm.go	69.58% (-2.06%)	240 (-21)	167 (-20)	73 (-1)	👎
github.com/NVIDIA/aicr/pkg/bundler/handler.go	73.60% (+0.65%)	125 (+3)	92 (+3)	33	👍
github.com/NVIDIA/aicr/pkg/cli/bundle.go	0.68% (ø)	146	1	145
github.com/NVIDIA/aicr/pkg/component/overrides.go	97.27% (+1.17%)	110 (-172)	107 (-164)	3 (-8)	👍

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.

mchmarny

Clean PR that achieves three things well: (1) CLI/API parity for --dynamic via the shared ComponentPath primitive, (2) replaces the fragile fixValuesTemplate string manipulation with proper yaml.Node LiteralStyle, and (3) removes ~470 lines of dead reflection-based ApplyValueOverrides code. CI all green, test coverage is thorough across the new ComponentPath parser, handler query param parsing, and the reworked ApplyMapOverrides tests.

One nit on the navigateParent / getOrCreateNestedMap near-duplication. Nothing blocks merge.

…ents The bundler --set flag validator rejected Helm-style array indexing such as "operator.tolerations[2].key", which is a standard Helm path expression. Pre-existing KWOK CI scripts rely on bracket indexing to append tolerations; without it every KWOK recipe validation fails at bundle generation before any recipe is exercised. Widen safePathPattern to allow an optional trailing "[N]" (digits only) per segment. The anti-injection intent is preserved: template expressions ({{ }}), path traversal (../), globs, and non-numeric bracket bodies are still rejected. Added direct-Parse tests covering acceptance of numeric indices and rejection of every unsafe bracket form. This regression was introduced by NVIDIA#603 (added the restrictive regex) but went undetected because the KWOK workflow only runs on changes under recipes/** and kwok/**, not pkg/**. Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

…ling.sh The `validate-scheduling.sh` script attempts to append a KWOK-test-specific toleration to `network-operator` via Helm-style `--set` array-index syntax: --set networkoperator:operator.tolerations[2].key=aicr.nvidia.com/kwok-test --set networkoperator:operator.tolerations[2].operator=Equal --set networkoperator:operator.tolerations[2].value=true --set networkoperator:operator.tolerations[2].effect=NoSchedule These lines have never worked. `aicr bundle --set` delegates path resolution to `pkg/component/overrides.go:getOrCreateNestedMap`, which splits the path on `.` only — `tolerations[2]` becomes a literal map key, not a list index, so the generated values emit `{tolerations[2]: {key: ..., ...}}` — a string key Helm silently ignores while keeping the upstream `tolerations` list's defaults. PR NVIDIA#603 later tightened `safePathPattern` to reject the `[N]` segment outright, so the failure turned from silent-wrong-bundle into loud-parse-error and KWOK Tier 1/2 CI jobs now fail at the bundle step. Drop the 4 dead `--set` lines. The adjacent `--accelerated-node-toleration kwok.x-k8s.io/node=fake:NoSchedule` already propagates a tolerate-the-fake- node rule to every accelerated workload (network-operator included) via the recipe's per-component nodeScheduling overlay, so nothing about the scheduling semantics of the KWOK test actually regresses — we only remove lines that were producing misleading bundle output to begin with. An additional negative test `helm-style array index rejected` documents why `foo:bar.tolerations[N].key=x` must stay rejected at parse time until someone teaches the downstream path walker to interpret `[N]` as a list index. That's a substantive feature (affects both `ApplyMapOverrides` and the dynamic-values extractor) and is intentionally out of scope here.

…ents The bundler --set flag validator rejected Helm-style array indexing such as "operator.tolerations[2].key", which is a standard Helm path expression. Pre-existing KWOK CI scripts rely on bracket indexing to append tolerations; without it every KWOK recipe validation fails at bundle generation before any recipe is exercised. Widen safePathPattern to allow an optional trailing "[N]" (digits only) per segment. The anti-injection intent is preserved: template expressions ({{ }}), path traversal (../), globs, and non-numeric bracket bodies are still rejected. Added direct-Parse tests covering acceptance of numeric indices and rejection of every unsafe bracket form. This regression was introduced by NVIDIA#603 (added the restrictive regex) but went undetected because the KWOK workflow only runs on changes under recipes/** and kwok/**, not pkg/**. Signed-off-by: Carlos Eduardo Arango Gutierrez <[email protected]>

…ling.sh The `validate-scheduling.sh` script attempts to append a KWOK-test-specific toleration to `network-operator` via Helm-style `--set` array-index syntax: --set networkoperator:operator.tolerations[2].key=aicr.nvidia.com/kwok-test --set networkoperator:operator.tolerations[2].operator=Equal --set networkoperator:operator.tolerations[2].value=true --set networkoperator:operator.tolerations[2].effect=NoSchedule These lines have never worked. `aicr bundle --set` delegates path resolution to `pkg/component/overrides.go:getOrCreateNestedMap`, which splits the path on `.` only — `tolerations[2]` becomes a literal map key, not a list index, so the generated values emit `{tolerations[2]: {key: ..., ...}}` — a string key Helm silently ignores while keeping the upstream `tolerations` list's defaults. PR NVIDIA#603 later tightened `safePathPattern` to reject the `[N]` segment outright, so the failure turned from silent-wrong-bundle into loud-parse-error and KWOK Tier 1/2 CI jobs now fail at the bundle step. Drop the 4 dead `--set` lines. The adjacent `--accelerated-node-toleration kwok.x-k8s.io/node=fake:NoSchedule` already propagates a tolerate-the-fake- node rule to every accelerated workload (network-operator included) via the recipe's per-component nodeScheduling overlay, so nothing about the scheduling semantics of the KWOK test actually regresses — we only remove lines that were producing misleading bundle output to begin with. An additional negative test `helm-style array index rejected` documents why `foo:bar.tolerations[N].key=x` must stay rejected at parse time until someone teaches the downstream path walker to interpret `[N]` as a list index. That's a substantive feature (affects both `ApplyMapOverrides` and the dynamic-values extractor) and is intentionally out of scope here.

lockwobr self-assigned this Apr 17, 2026

github-actions Bot added area/bundler area/cli area/api size/XL labels Apr 17, 2026

lockwobr force-pushed the feat/api-dynamic branch 2 times, most recently from c6292aa to 29c56cb Compare April 20, 2026 21:56

feat(bundler/api): add --dynamic query parameter to bundle handler

93f77c5

lockwobr force-pushed the feat/api-dynamic branch from 29c56cb to 93f77c5 Compare April 20, 2026 22:38

lockwobr marked this pull request as ready for review April 20, 2026 22:47

lockwobr requested a review from a team as a code owner April 20, 2026 22:47

mchmarny approved these changes Apr 20, 2026

View reviewed changes

Comment thread pkg/bundler/config/component_path.go

Comment thread pkg/bundler/deployer/argocdhelm/argocdhelm.go

Comment thread pkg/bundler/deployer/argocdhelm/argocdhelm.go

Comment thread pkg/bundler/handler.go

Comment thread pkg/component/overrides.go

Comment thread api/aicr/v1/server.yaml

lockwobr merged commit f1212ad into main Apr 20, 2026
25 checks passed

lockwobr deleted the feat/api-dynamic branch April 20, 2026 23:02

coderabbitai Bot mentioned this pull request Apr 21, 2026

feat(bundler): enable --attest and --data for argocd-helm (#573) #627

Merged

25 tasks

This was referenced Apr 22, 2026

fix(kwok): remove broken --set array-index lines from validate-scheduling.sh #643

Merged

feat(validator): add inference performance validation #641

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(bundler/api): add --dynamic query parameter to bundle handler#603

feat(bundler/api): add --dynamic query parameter to bundle handler#603
lockwobr merged 1 commit into
mainfrom
feat/api-dynamic

lockwobr commented Apr 17, 2026

Uh oh!

coderabbitai Bot commented Apr 17, 2026 •

edited

Loading

Walkthrough

Estimated code review effort

Detailed Notes

Uh oh!

github-actions Bot commented Apr 17, 2026 •

edited

Loading

Changed files (no unit tests)

Uh oh!

mchmarny left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

lockwobr commented Apr 17, 2026

Summary

Motivation / Context

Type of Change

Component(s) Affected

Implementation Notes

Testing

Risk Assessment

Checklist

Uh oh!

coderabbitai Bot commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Detailed Notes

Uh oh!

github-actions Bot commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report ✅

Merging this branch changes the coverage (2 decrease, 2 increase)

Changed files (no unit tests)

Uh oh!

mchmarny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Apr 17, 2026 •

edited

Loading

github-actions Bot commented Apr 17, 2026 •

edited

Loading