feat(chart): add values.schema.json for Helm values validation - #2553
feat(chart): add values.schema.json for Helm values validation#2553Rickydama3 wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Draft-07 JSON Schema for the HAMi Helm chart. The schema validates chart settings, scheduler and admission configuration, device plugin values, ports, optional integrations, and supported device enablement flags. ChangesHelm values validation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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
🤖 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 `@charts/hami/values.schema.json`:
- Around line 41-47: Update the deviceMemoryScaling and deviceCoreScaling
definitions to use oneOf: numeric values must be numbers with a minimum of 0,
while string values must retain the existing non-negative decimal pattern.
Remove the shared type/pattern structure so numeric -1 cannot pass validation.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a8a86c5a-ae44-49d3-8cdb-33510741cf9e
📒 Files selected for processing (1)
charts/hami/values.schema.json
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
c58a383 to
cb51eee
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@charts/hami/values.schema.json`:
- Around line 20-32: Update values.schema.json so
admissionWebhook.customURL.port is defined at the top-level path consumed by
values.yaml, rather than only under scheduler.admissionWebhook. Add the existing
service port definitions under scheduler.service to match
templates/scheduler/service.yaml, while retaining devicePlugin.service
validation for its own consumers.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2319c834-a713-4670-b4ed-8f77a52d3752
📒 Files selected for processing (1)
charts/hami/values.schema.json
cb51eee to
255400b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@charts/hami/values.schema.json`:
- Around line 5-7: Update the schemas for metaxsGPUTopologyAware and the other
corresponding string-typed boolean fields to restrict values with an enum
containing only the chart-supported boolean text values, true and false.
Preserve their string type while rejecting values such as enabled and 0.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 16d6ad8b-075f-4e4e-9ac6-327d09813bea
📒 Files selected for processing (1)
charts/hami/values.schema.json
255400b to
cfac946
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@charts/hami/values.schema.json`:
- Around line 38-52: Update the scheduler Service port properties httpPort,
schedulerPort, monitorPort, monitorTargetPort, and httpTargetPort in the schema
to enforce valid port numbers by adding minimum 1 and maximum 65535 constraints
to each integer definition.
- Around line 14-55: Update charts/hami/values.schema.json at lines 14-55 and
57-113: add boolean type definitions for
scheduler.forceOverwriteDefaultScheduler, scheduler.livenessProbe,
scheduler.leaderElect, scheduler.kubeScheduler.enabled, devicePlugin.enabled,
devicePlugin.hostPID, devicePlugin.hostNetwork, and
devicePlugin.gpuOperatorToolkitReady.enabled. Preserve the existing schema
structure and unknown-key behavior while ensuring these documented fields reject
string values.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa01367e-e48c-4d15-ad78-315ad2c3c995
📒 Files selected for processing (1)
charts/hami/values.schema.json
cfac946 to
56415bd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@charts/hami/values.schema.json`:
- Around line 36-38: Update the replicas schema definition to enforce a minimum
value of 0, rejecting negative replica counts while preserving zero as valid.
Modify the integer constraint within the replicas property only.
- Around line 14-35: Add schemas in charts/hami/values.schema.json for
scheduler.defaultSchedulerPolicy.nodeSchedulerPolicy,
scheduler.defaultSchedulerPolicy.gpuSchedulerPolicy, and
scheduler.metricsBindAddress as strings, and scheduler.admissionWebhook.enabled
as a boolean; update both the scheduler properties at lines 14-35 and the
related scheduler section at lines 39-50 as needed, ensuring string values such
as "false" are not used for boolean Helm conditionals.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b26ff705-94ad-409e-ba13-9d47615910df
📒 Files selected for processing (1)
charts/hami/values.schema.json
56415bd to
61a5ad7
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: leodon33 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…roject-HAMi#2419) Signed-off-by: Ricky Dama <rickydama2006@gmail.com>
cd9ef8d to
7dd4dee
Compare
|
This is being closed because it does not comply with the contribution guidelines. |
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
Adds
charts/hami/values.schema.json— a JSON Schema (draft-07) that Helm 3.x automatically validates the coalesced chart values against duringhelm lint,helm template, andhelm install/upgrade. Today the chart ships no schema, so type errors in user-supplied values (a quoted integer, a bool where a string is expected, a non-null value on a null-guarded field) surface only as a broken render or a malformed generated config at install time.Which issue(s) this PR fixes:
Fixes #2419
Special notes for your reviewer:
Design
The schema is deliberately permissive. It constrains only load-bearing types, each traced to how the templates actually consume the value:
scheduler.overwriteEnv,metaxsGPUTopologyAware,devicePlugin.disablecorelimit) — rendered as raw YAML scalars / CLI flags, so typedstring, notboolean.nvidiaHookPath/nvidiaDriverRoot→["null","string"];gdrcopyEnabled/gdsEnabled/mofedEnabled→["null","boolean"], matching thetypeIsguards indaemonsetnvidia.yaml.deviceSplitCount,preConfiguredDeviceMemory,scheduler.replicas,ports— quoted strings are correctly rejected.deviceMemoryScaling/deviceCoreScalingaccept a number or a numeric string (^[0-9]+(\.[0-9]+)?$). They are Go*float64, so fractional values are first-class; the string branch preserves--set devicePlugin.deviceMemoryScaling=1.5(Helm renders--setfloats as strings) while still rejecting non-numeric input.nodeSchedulerPolicy/gpuSchedulerPolicyare not enum-constrained (topology-awareis valid).devices.amd/devices.awsneuroncarry noenabledkey (their customresources are ranged unconditionally); other vendors keepenabled.No
additionalProperties: falseand an empty top-levelrequired, so undocumented and forward-compatible keys and every documented install path keep working.Testing
Validated with the CI-pinned Helm v3.7.1:
helm lint charts/hami→ 0 failures;helm templaterenders all manifests.--setscaling (1.5/0.8/2), genuine floats via a values file, and every--setoverride used byhack/deploy-helm.sh(empty registries,leaderElect=false) all pass.deviceSplitCount,replicas) all correctly rejected.No Go files changed; build/lint/test are unaffected.
Does this PR introduce a user-facing change?: