Conversation
Signed-off-by: Arash Javanmardi <arash@javanmardi.info>
📝 WalkthroughWalkthroughThe DCGM, DCGM exporter, and DRA validator DaemonSet templates now use Priority: ➖ Normal Merge Risk: 🔵 Low · up to Clusters using YAML-keyword priority-class names can fail to render the affected DaemonSets correctly. Quote the configured value and add the regression case before merging. Comment |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
manifests/state-dcgm/0500_daemonset.yaml-39-39 (1)
39-39: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winQuote the defaulted
PriorityClassNamein all three templates.DaemonsetsSpec.PriorityClassNameis a string, and values such as"on","off","true", and"null"are valid string values. The renderer decodes these templates through Kubernetes YAML-to-JSON conversion. Unquotedon,off, andtruebecome boolean values, whilenullbecomes nil. Typed DaemonSet conversion then rejects the boolean values and loses the configured"null"value.Apply the same fix at all three sites:
manifests/state-dcgm/0500_daemonset.yaml#L39manifests/state-dcgm-exporter/0700_daemonset.yaml#L42manifests/state-dra-validation/0500_daemonset.yaml#L59priorityClassName: {{ .Daemonsets.PriorityClassName | default "system-node-critical" | quote }}Add an
"on"case toTestGPUClusterDaemonsetsPriorityClass. The test exercises all three renderers and converts the rendered object to a typed DaemonSet, so it will fail without the quote and assert that the value remains"on"with the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 8c6b81a7-7df7-4de8-954b-112d646718c9
📒 Files selected for processing (4)
internal/state/gpucluster_render_test.gomanifests/state-dcgm-exporter/0700_daemonset.yamlmanifests/state-dcgm/0500_daemonset.yamlmanifests/state-dra-validation/0500_daemonset.yaml
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.
Signed-off-by: Arash Javanmardi <arash@javanmardi.info>
|
Thanks for the contribution. We will review this PR soon. |
Description
Setting
GPUCluster.spec.daemonsets.priorityClassNameleft DCGM, DCGM exporter, and DRA validator atsystem-node-critical, although their render data already carried the requested value. Honor the configured name in these three DaemonSets, preservingsystem-node-criticalwhen unset.Quote the resolved value so names such as
onandnullremain strings through YAML decoding. Cover ordinary custom names, an unset value, a YAML boolean keyword, and a YAML null keyword through the real renderer for all three operands (12 cases). Existing golden manifests remain unchanged.Checklist
make lint)make validate-generated-assets)make validate-modules)Testing
on/nullcases failed while the six existing custom/default cases passed. With quoting, all 12 cases pass through real rendering and typed DaemonSet conversion.make fmt license-check: passed.make coverage: passed, including build and the full unit suite (21 tested packages); existing golden manifests unchanged.make lint: passed with zero issues.make validate-modules: passed for the source module, vendor tree, and tools module.make validate-generated-assets: passed; no generated-file drift.make check-third-party-notices: passed for all 123 packages on Linux AMD64 and ARM64; notices unchanged.