feat(bundler): add --storage-class flag for registry-driven injection#729
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/user/cli-reference.md`:
- Around line 904-905: Update the CLI docs to explicitly state that an explicit
per-component --set <component>:<path>=... on the same storageClassPaths path
takes precedence over the global --storage-class; reference the implementation
behavior in pkg/bundler/bundler.go (the logic around Lines 651-672) to ensure
wording matches code, and apply the same clarification to the duplicate sentence
at the other instance (around lines 920-921) so both locations document that
--set overrides --storage-class.
In `@pkg/bundler/bundler_test.go`:
- Around line 909-911: The test currently uses t.Skip when the registry contract
for kube-prometheus-stack or storageClassPaths is missing (checks: comp == nil
|| len(comp.GetStorageClassPaths()) == 0); change these to fail fast using
t.Fatalf with a clear message (e.g., "registry missing kube-prometheus-stack or
storageClassPaths: cannot run storage class path test") so CI fails instead of
silently skipping; apply the same replacement for the second occurrence around
the 976-978 check to ensure missing registry entries cause test failures rather
than skips.
- Around line 901-1009: Replace the three separate tests
(TestApplyNodeSchedulingOverrides_StorageClass,
TestApplyNodeSchedulingOverrides_StorageClass_Empty,
TestApplyNodeSchedulingOverrides_StorageClass_ExplicitOverridePrecedence) with a
single table-driven test that enumerates the scenarios: (1) global storageClass
set -> inject into paths, (2) global storageClass empty -> do not override
existing values, and (3) explicit per-component --set override wins over global
storageClass. For each case set up inputs using config.NewConfig (with/without
WithStorageClass and WithValueOverrides), create the bundle via New(WithConfig),
prepare the values map (use component.ApplyMapOverrides when simulating CLI
--set), call b.applyNodeSchedulingOverrides("kube-prometheus-stack", values),
and assert with component.GetValueByPath that the final value matches the
expected result; use t.Run for subtests and include the registry/comp presence
check (recipe.GetComponentRegistry and registry.Get) to t.Skip when
storageClassPaths are unavailable.
In `@pkg/bundler/config/config.go`:
- Around line 134-136: Change the optional storageClass field from string to
*string in the config struct (symbol: storageClass) and apply the same
pointer-pattern to the other optional fields flagged (the ones around the same
declaration and usages). Update all uses of storageClass (e.g., any
StorageClass() accessor, defaulting/validation logic, NewConfig or unmarshalling
code that sets/reads storageClass, and places that compare it to "" or assign
"") to handle nil vs non-nil (check for nil, and dereference *storageClass when
you need the string value); adjust JSON/YAML marshal/unmarshal or helper getters
so nil means “unset” and &"" means “set to empty” per the repo convention.
Ensure call sites that previously relied on empty string to mean unset are
changed to nil checks or use a helper like GetStorageClass() that returns
(string, bool) or *string as appropriate.
In `@pkg/cli/bundle.go`:
- Around line 344-348: The new CLI flag "storage-class" added in the StringFlag
list is missing from the single-value flag enforcement, so update the validation
list in validateSingleValueFlags to include "storage-class" (the exact string
name) alongside the other single-value flag names; locate
validateSingleValueFlags and add "storage-class" to the slice/array of flag
names it checks so repeated --storage-class usages will be rejected like the
other single-value flags.
🪄 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: 48694af9-bd24-4570-a04a-2a0f21d7c169
📒 Files selected for processing (8)
docs/user/cli-reference.mdpkg/bundler/bundler.gopkg/bundler/bundler_test.gopkg/bundler/config/config.gopkg/bundler/config/config_test.gopkg/cli/bundle.gopkg/recipe/components.gorecipes/registry.yaml
| // storageClass is the Kubernetes StorageClass name to inject into components at bundle time. | ||
| // When non-empty, it overrides the storageClassName at all registry-declared storageClassPaths. | ||
| storageClass string |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Use pointer semantics for optional storageClass config
Lines 134-136 and Lines 484-490 model an optional value as string, which conflates “unset” and “set to empty” and diverges from the repo’s optional-config convention.
♻️ Proposed refactor
- storageClass string
+ storageClass *string
...
-func (c *Config) StorageClass() string {
- return c.storageClass
+func (c *Config) StorageClass() *string {
+ if c.storageClass == nil {
+ return nil
+ }
+ v := *c.storageClass
+ return &v
}
...
func WithStorageClass(storageClass string) Option {
return func(c *Config) {
- c.storageClass = storageClass
+ if storageClass == "" {
+ return
+ }
+ sc := storageClass
+ c.storageClass = &sc
}
}As per coding guidelines: “Use pointer pattern for optional config (nil = not set, &value = set).”
Also applies to: 292-295, 484-490
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/bundler/config/config.go` around lines 134 - 136, Change the optional
storageClass field from string to *string in the config struct (symbol:
storageClass) and apply the same pointer-pattern to the other optional fields
flagged (the ones around the same declaration and usages). Update all uses of
storageClass (e.g., any StorageClass() accessor, defaulting/validation logic,
NewConfig or unmarshalling code that sets/reads storageClass, and places that
compare it to "" or assign "") to handle nil vs non-nil (check for nil, and
dereference *storageClass when you need the string value); adjust JSON/YAML
marshal/unmarshal or helper getters so nil means “unset” and &"" means “set to
empty” per the repo convention. Ensure call sites that previously relied on
empty string to mean unset are changed to nil checks or use a helper like
GetStorageClass() that returns (string, bool) or *string as appropriate.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/bundler/config/config.go (1)
134-136: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftModel optional
storageClasswith pointer semantics (nilvs set).Using
stringhere collapses “unset” and “set empty,” which diverges from the repo’s optional-config pattern.♻️ Proposed direction
- storageClass string + storageClass *string - func (c *Config) StorageClass() string { - return c.storageClass + func (c *Config) StorageClass() *string { + if c.storageClass == nil { + return nil + } + v := *c.storageClass + return &v } func WithStorageClass(storageClass string) Option { return func(c *Config) { - c.storageClass = storageClass + if storageClass == "" { + return + } + sc := storageClass + c.storageClass = &sc } }As per coding guidelines: “Use pointer pattern for optional config (nil = not set, &value = set). Avoid boolean flags to track whether option was set.”
Also applies to: 292-295, 484-490
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/bundler/config/config.go` around lines 134 - 136, Change the storageClass field in the config struct from string to *string to model "unset" vs "set (possibly empty)"; update all code that constructs, reads, compares or serializes storageClass to handle pointer semantics (treat nil = not set, &"" = explicitly set empty) including any setters/getters/validation and the code paths that override storageClassName / registry-declared storageClassPaths where storageClass is referenced; ensure assignments create a pointer (e.g., take address of value) and all comparisons dereference safely (check nil before use) and update JSON/YAML (un)marshalling or defaulting logic to preserve nil vs set-empty semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/bundler/config/config.go`:
- Around line 134-136: Change the storageClass field in the config struct from
string to *string to model "unset" vs "set (possibly empty)"; update all code
that constructs, reads, compares or serializes storageClass to handle pointer
semantics (treat nil = not set, &"" = explicitly set empty) including any
setters/getters/validation and the code paths that override storageClassName /
registry-declared storageClassPaths where storageClass is referenced; ensure
assignments create a pointer (e.g., take address of value) and all comparisons
dereference safely (check nil before use) and update JSON/YAML (un)marshalling
or defaulting logic to preserve nil vs set-empty semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1198ce93-b34a-4eb1-81ac-aef228c93e0d
📒 Files selected for processing (8)
docs/user/cli-reference.mdpkg/bundler/bundler.gopkg/bundler/bundler_test.gopkg/bundler/config/config.gopkg/bundler/config/config_test.gopkg/cli/bundle.gopkg/recipe/components.gorecipes/registry.yaml
|
The finding is not needed. Here's the verification: Semantic soundness: "" == unset is correct for storage class. Kubernetes StorageClass names follow RFC 1123 DNS subdomain rules — an empty string is never a valid value, so there is no ambiguity between "unset" and "explicitly set to empty." Codebase consistency: Every other optional string in this struct uses the same pattern: repoURL string — "" means unset CLAUDE.md anti-pattern: The rule says "Use pointer pattern instead of boolean flags to track options." That applies when you'd otherwise write storageClass string + storageClassSet bool side by side. The current code has no such boolean companion — the empty-string sentinel is the pattern used by every other field. Call sites: bundler.go:654 correctly guards with sc != "". cli/bundle.go:473 passes cmd.String(...) which returns "" when unset — exactly right. Changing to *string here would break the existing pattern without providing any semantic benefit. No change needed. |
|
Welcome to AICR, and thanks for picking this up — exactly the kind of cleanup the project benefits from. One scope question to settle before a line-level pass. PR scope vs. Issue #503The mechanism here is the right shape — registry-driven What's missing is the actual decoupling. Issue #503 outlines the need for better decoupling of storageClass from recipe overlays (solving the hardcoded values scattered across overlay files). Ideally we would want not propagate that while making changes in this area:
As-is, the PR adds an override path but the overlays remain CSP-specific on this dimension, and the "blocks further mixin extraction" benefit the issue cites is unchanged. PTAL at issue #503 with this lens and extend the PR to remove those four entries. It's a small delta on top of what's already here and would let this PR actually close the issue end-to-end. Smaller observations
Once scope is settled I'll do a proper line-level review. Thanks again. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@dtzar the PR looks good, you will have to amend the commit though with verified signature. See: https://github.com/NVIDIA/aicr/blob/main/CONTRIBUTING.md#developer-certificate-of-origin |
dtzar
force-pushed
the
main
branch
2 times, most recently
from
c1aeac7 to
ef09b9f
Compare
Signed-off-by: David Tesar <[email protected]>
2 participants
Summary
Decouples storageClass from recipe overlays
Motivation / Context
Fixes: #503
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/api,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
Implemented with Claude Code assistance; all code reviewed and tested locally (make qualify passes).
Testing
# Commands run (prefer `make qualify` for non-trivial changes) make qualifyRisk Assessment
The change is purely additive — a new optional flag that's a no-op when absent. No existing behavior changes: overlay defaults still apply, nothing is removed or reordered.
Checklist
make testwith-race)make lint)git commit -S) — GPG signing info