feat(chart): add values.schema.json for Helm values validation - #2441
feat(chart): add values.schema.json for Helm values validation#2441im-Toqeer-506 wants to merge 1 commit into
Conversation
Add JSON Schema (draft-07) to validate Helm chart values at lint/template/install time. Constrains load-bearing types (null-or-string unions, string-typed booleans, integers, float scaling factors) grounded in actual template usage. Permissive baseline: no additionalProperties: false, empty required. Bumps chart version to 2.9.1. Fixes #<issue-number> Signed-off-by: M Toqeer Zia <muhammadtoqeerzia586694@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: im-Toqeer-506 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 |
📝 WalkthroughWalkthroughThe HAMi Helm chart adds a permissive values schema with typed nested settings and updates both chart and application versions from 2.9.0 to 2.9.1. ChangesHAMi Helm chart
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
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: 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/Chart.yaml`:
- Around line 20-24: Align the Chart.yaml appVersion with the released HAMi
image version used by VERSION, global.imageTag, README, and CHANGELOG; retain
v2.9.0 unless this change also introduces an available v2.9.1 release, and
ensure the corresponding imageTag default matches.
In `@charts/hami/values.schema.json`:
- Around line 227-230: Add a minimum constraint of 0 to both
devicePlugin.deviceMemoryScaling at charts/hami/values.schema.json lines 227-230
and devicePlugin.deviceCoreScaling at lines 232-235, preserving their existing
number/string types and numeric string patterns.
🪄 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: 8a8a0007-6ff8-4b2f-9929-0afbe9671bae
📒 Files selected for processing (2)
charts/hami/Chart.yamlcharts/hami/values.schema.json
| # This is the version number of the application being deployed. This version number should be | ||
| # incremented each time you make changes to the application. Versions are not expected to | ||
| # follow Semantic Versioning. They should reflect the version the application is using. | ||
| # It is recommended to use it with quotes. | ||
| appVersion: "2.9.0" | ||
| appVersion: "2.9.1" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 'Chart\.AppVersion|appVersion|image:|repository:|tag:' charts/hami
rg -n '2\.9\.[01]' charts/hamiRepository: Project-HAMi/HAMi
Length of output: 13398
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Chart.yaml ---'
cat -n charts/hami/Chart.yaml | sed -n '1,80p'
printf '%s\n' '--- helpers image definitions ---'
cat -n charts/hami/templates/_helpers.tpl | sed -n '1,180p'
printf '%s\n' '--- version/imageTag references in repo ---'
rg -n 'v2\.9\.[01]|2\.9\.[01]|global\.imageTag|imageTag:' .Repository: Project-HAMi/HAMi
Length of output: 10655
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- Helm chart metadata references ---'
rg -n 'hami\.imageTagOrGlobal|hami\.image\(|Chart\.AppVersion|global\.imageTag|appVersion|v2\.9\.0|v2\.9\.1|imageTag' charts/hami hack docs CHANGELOG.md VERSION README.md
printf '%s\n' '--- Chart values defaults ---'
python3 - <<'PY'
from pathlib import Path
p=Path('charts/hami/values.yaml')
text=p.read_text()
for i,line in enumerate(text.splitlines(),1):
if 'imageTag:' in line:
print(f'{i}: {line}')
PYRepository: Project-HAMi/HAMi
Length of output: 1860
Align appVersion with the released HAMi image tag.
VERSION, global.imageTag, README, and the CHANGELOG still point to HAMi v2.9.0, while appVersion is bumped to 2.9.1 without an explicit v2.9.1 default. Set appVersion/imageTag to an available HAMi release, or keep them at v2.9.0 unless this PR also introduces that release.
🤖 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 `@charts/hami/Chart.yaml` around lines 20 - 24, Align the Chart.yaml appVersion
with the released HAMi image version used by VERSION, global.imageTag, README,
and CHANGELOG; retain v2.9.0 unless this change also introduces an available
v2.9.1 release, and ensure the corresponding imageTag default matches.
| "deviceMemoryScaling": { | ||
| "type": ["number", "string"], | ||
| "pattern": "^[0-9]+(\\.[0-9]+)?$", | ||
| "description": "Memory over-subscription ratio, parsed as a float64 (fractional values such as 0.8 or 1.5 are valid). Accepts a number from a values file, or a numeric string because Helm renders floats passed via --set as strings; the numeric pattern still rejects non-numeric input." |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== schema excerpt =="
sed -n '220,240p' charts/hami/values.schema.json
echo
echo "== schema draft/version hints =="
python3 - <<'PY'
import json
from pathlib import Path
p=Path("charts/hami/values.schema.json")
try:
data=json.loads(p.read_text())
print("$schema:", data.get("$schema"))
print("additionalProperties:", data.get("additionalProperties"))
except Exception as e:
print("error:", e)
PY
echo
echo "== relevant schema snippets using field names =="
rg -n '"deviceMemoryScaling"|"deviceCoreScaling"|minimum|pattern' charts/hami/values.schema.jsonRepository: Project-HAMi/HAMi
Length of output: 1923
🌐 Web query:
JSON Schema Draft 07 validation keywords for strings pattern does not apply to non string instances additionalProperties true
💡 Result:
In JSON Schema Draft 07, the "pattern" keyword is a string validation keyword [1][2]. By definition, this keyword applies only to instances that are JSON strings [1][3]. If an instance is of a different type (such as a number, boolean, object, or array), the "pattern" keyword is ignored, and the instance is considered valid against that keyword [4][3]. This behavior is consistent with the design of JSON Schema as a constraint-driven language [4]. Validation keywords generally only perform checks if the instance matches the expected type; otherwise, they treat the instance as valid [3]. If you intend to restrict an instance to a string that must also match a specific pattern, you must explicitly use the "type" keyword in your schema [4]: { "type": "string", "pattern": "^[a-z]+$" } In this example, the "type": "string" constraint ensures that only strings are accepted, while the "pattern" constraint further restricts those strings to the specified regular expression [4][3]. If you omit the "type": "string" constraint, the "pattern" keyword will allow any non-string instance to pass validation [4][3].
Citations:
- 1: https://json-schema.org/draft-07/json-schema-validation
- 2: https://www.learnjsonschema.com/draft7/validation/
- 3: https://jsonic.io/guides/json-schema-validation-keywords
- 4: https://www.learnjsonschema.com/draft7/validation/pattern/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import json
schema = json.loads(Path("charts/hami/values.schema.json").read_text())
def path_of(name):
p = []
obj = schema
for part in name.split("."):
if isinstance(part, int):
p.append(part); obj = obj[part]
else:
p.append(part); obj = obj["properties"][part]
return p
fields = ["deviceMemoryScaling", "deviceCoreScaling"]
for name in fields:
s = schema
parts = name.split(".")
for part in parts:
s = s["properties"][part]
print(name, "type=", s.get("type"), "pattern=", s.get("pattern"), "minimum=", "minimum" in s, "allOf=", "allOf" in s)
PYRepository: Project-HAMi/HAMi
Length of output: 261
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import json
schema = json.loads(Path("charts/hami/values.schema.json").read_text())
found = False
def walk(obj, prefix=""):
global found
if isinstance(obj, dict):
if $schema := obj.get("$schema"):
print("$schema:", $schema)
for k, v in obj.items():
path = f"{prefix}.{k}" if prefix else k
if isinstance(v, dict):
types = v.get("type")
patterns = v.get("pattern")
mins = "minimum" in v
if "deviceMemoryScaling" in k or "deviceCoreScaling" in k:
print("FOUND", path, "type=", types, "pattern=", patterns, "minimum_exists=", mins)
found = True
walk(v, path)
elif isinstance(obj, list):
for i, v in enumerate(obj):
path = f"{prefix}[{i}]"
walk(v, path)
walk(schema)
print("seen_found_at_top_level_properties:", "deviceMemoryScaling" in schema.get("properties", {}), "deviceCoreScaling" in schema.get("properties", {}))
PY
echo
echo "== deterministic negative-number evidence using JSON-schema examples from docs =="
python3 - <<'PY'
import json
from pathlib import Path
schema = {"$schema":"http://json-schema.org/draft-07/schema#","type":"object","properties":{"field":{"type":["number","string"],"pattern":"^[0-9]+(\\.[0-9]+)?$"}, "schema": schema}}
# Print the minimal schema shape for the fields in question with context.
PYRepository: Project-HAMi/HAMi
Length of output: 255
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import json
schema = json.loads(Path("charts/hami/values.schema.json").read_text())
schema_ref = schema.get("$schema")
print("$schema:", schema_ref)
print("top-level properties contain deviceMemoryScaling:", schema.get("properties", {}).get("deviceMemoryScaling", _ := _).__class__.__name__ if False else ("deviceMemoryScaling" in schema.get("properties", {})))
print("top-level properties contain deviceCoreScaling:", "deviceCoreScaling" in schema.get("properties", {}))
def walk(obj, prefix=""):
if isinstance(obj, dict):
for k, v in obj.items():
path = (prefix + "." + k) if prefix else k
if isinstance(v, dict):
if "deviceMemoryScaling" in k or "deviceCoreScaling" in k:
print("FOUND", path, "type=", v.get("type"), "pattern=", v.get("pattern"), "has_minimum=", "minimum" in v)
walk(v, path)
elif isinstance(obj, list):
for i, v in enumerate(obj):
walk(v, prefix + f"[{i}]")
walk(schema)
with open("/tmp/negative_scaling_inputs.json", "w") as f:
json.dump({
"deviceMemoryScaling": -1.0,
"deviceCoreScaling": -0.5,
}, f)
PY
echo
echo "== values near these fields in repo files =="
rg -n -C 2 'deviceMemoryScaling|deviceCoreScaling' charts/hami . --glob '*.yaml' --glob '*.yml' --glob '*.json' 2>/dev/null || trueRepository: Project-HAMi/HAMi
Length of output: 5463
Apply a numeric lower bound to both scaling fields.
This schema uses Draft-07, and pattern only validates string values. With type: ["number", "string"], negative numeric values such as -1.5 pass these fields. Add "minimum": 0 to both devicePlugin.deviceMemoryScaling and devicePlugin.deviceCoreScaling.
📍 Affects 1 file
charts/hami/values.schema.json#L227-L230(this comment)charts/hami/values.schema.json#L232-L235
🤖 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 `@charts/hami/values.schema.json` around lines 227 - 230, Add a minimum
constraint of 0 to both devicePlugin.deviceMemoryScaling at
charts/hami/values.schema.json lines 227-230 and devicePlugin.deviceCoreScaling
at lines 232-235, preserving their existing number/string types and numeric
string patterns.
|
Pls read the rules first. |
2 participants
What this PR does
Adds
charts/hami/values.schema.json— a JSON Schema (draft-07) that Helm 3.xautomatically validates the coalesced chart values against during
helm lint,helm template, andhelm install/upgrade. Today the chart ships no schema, sotype 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.
Also bumps the chart
version/appVersion2.9.0→2.9.1(patch: abackward-compatible addition), keeping them equal as
hack/verify-chart-version.shrequires.
Fixes ##2419
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, sotyped
string, notboolean.nvidiaHookPath/nvidiaDriverRoot→["null","string"];gdrcopyEnabled/gdsEnabled/mofedEnabled→["null","boolean"], matchingthe
typeIsguards indaemonsetnvidia.yaml.deviceSplitCount,preConfiguredDeviceMemory,scheduler.replicas, ports — quoted strings are correctly rejected.deviceMemoryScaling/deviceCoreScalingaccept anumber or a numeric string (
^[0-9]+(\.[0-9]+)?$). They are Go*float64, sofractional values are first-class; the string branch preserves
--set devicePlugin.deviceMemoryScaling=1.5(Helm renders--setfloats asstrings) while still rejecting non-numeric input.
nodeSchedulerPolicy/gpuSchedulerPolicyare notenum-constrained (
topology-awareis valid).devices.amd/devices.awsneuroncarry noenabledkey (theircustomresourcesare ranged unconditionally); other vendors keepenabled.No
additionalProperties: falseand an empty top-levelrequired, so undocumentedand 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 avalues file, and every
--setoverride used byhack/deploy-helm.sh(empty registries,
leaderElect=false,passDeviceSpecsEnabled=false) all pass.(
deviceSplitCount,replicas) all correctly rejected.No Go files changed; build/lint/test are unaffected.
Summary by CodeRabbit
New Features
Chores