Skip to content

fix(power-agent): keep apiVersion off SPDX comment line - #12651

Merged
kaim-eng merged 2 commits into
mainfrom
fix/power-agent-helm-apiVersion-whitespace
Aug 4, 2026
Merged

fix(power-agent): keep apiVersion off SPDX comment line#12651
kaim-eng merged 2 commits into
mainfrom
fix/power-agent-helm-apiVersion-whitespace

Conversation

@kaim-eng

@kaim-eng kaim-eng commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fixes DYN-3741 / NVBug 6555989: default helm install of deploy/helm/charts/power-agent failed with apiVersion not set because a trailing whitespace-trim (-}}) on the silent validateTerminationGracePeriod include glued apiVersion onto the SPDX comment (Apache-2.0apiVersion: apps/v1).
  • Drop that trailing - in daemonset.yaml and dev-pod.yaml (Fix A).
  • Add isAPIVersion helm-unittest coverage. helm template / helm lint exit 0 on the broken render; the previous 48 unittests stayed green because nothing asserted apiVersion.

Validation

  • helm template ... --set image.tag=v1.4.0 emits standalone apiVersion: apps/v1 (DaemonSet) and apiVersion: v1 (dev pod)
  • make lint in deploy/helm/charts/power-agent
  • make test — 50 passed (incl. new api_version_whitespace_test.yaml)

Related


Open in Devin Review

Summary by CodeRabbit

  • Bug Fixes
    • Fixed Helm chart rendering to preserve the apiVersion field correctly in DaemonSet and development Pod manifests.
    • Added regression coverage to verify valid rendered Kubernetes resources and prevent whitespace-related manifest issues.

Drop the trailing whitespace-trim on the silent
validateTerminationGracePeriod include so helm does not glue
apiVersion onto the SPDX comment (DYN-3741 / NVBug 6555989).
Add isAPIVersion helm-unittest coverage; template/lint stay green
on the broken render and the old suite did not catch it.

Signed-off-by: Kai Ma <kaim@nvidia.com>
@kaim-eng
kaim-eng requested a review from a team as a code owner August 4, 2026 18:50
@copy-pr-bot

copy-pr-bot Bot commented Aug 4, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added deployment::k8s Relates to dynamo deployment in kubernetes fix labels Aug 4, 2026

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Open in Devin Review

Comment thread deploy/helm/charts/power-agent/tests/api_version_whitespace_test.yaml Outdated
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

The Helm DaemonSet and development Pod templates now preserve whitespace around termination grace-period validation. A new unittest suite verifies that rendered resources retain valid apiVersion document keys.

Changes

Helm template rendering

Layer / File(s) Summary
Preserve API version rendering
deploy/helm/charts/power-agent/templates/daemonset.yaml, deploy/helm/charts/power-agent/templates/dev-pod.yaml, deploy/helm/charts/power-agent/tests/api_version_whitespace_test.yaml
The templates stop trimming whitespace after termination grace-period validation. Tests verify the rendered resource kinds and API versions.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main fix: preventing apiVersion from being concatenated with the SPDX comment line.
Description check ✅ Passed The description explains the issue, changes, validation, and related references, but it does not include a separate reviewer-start section.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@deploy/helm/charts/power-agent/tests/api_version_whitespace_test.yaml`:
- Around line 4-5: Replace the internal ticket reference “DYN-3741” in the
regression comment with its mapped GitHub issue reference from
.ai/linear-ticket-refs.md, preserving the rest of the comment unchanged.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 77a41f27-e92b-46ff-8be1-155d0419f01b

📥 Commits

Reviewing files that changed from the base of the PR and between 9a7b29e and 4bb3761.

📒 Files selected for processing (3)
  • deploy/helm/charts/power-agent/templates/daemonset.yaml
  • deploy/helm/charts/power-agent/templates/dev-pod.yaml
  • deploy/helm/charts/power-agent/tests/api_version_whitespace_test.yaml

Comment thread deploy/helm/charts/power-agent/tests/api_version_whitespace_test.yaml Outdated
Rewrite the regression-test comment without Linear/NVBug IDs and
bump the chart patch version so the install-blocking apiVersion fix
is a distinct chart artifact on main and release/1.4.0.

Signed-off-by: Kai Ma <kaim@nvidia.com>
@kaim-eng

kaim-eng commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

/ok to test cafb4f5

@datadog-official

This comment has been minimized.

@dagil-nvidia dagil-nvidia left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. Verified for 1.4.0 release triage (NVBug 6555989 / DYN-3741, P1).

  • Root cause is correctly identified: the -}} right-trim on the silent validateTerminationGracePeriod include consumed the newline before apiVersion, rendering # SPDX-License-Identifier: Apache-2.0apiVersion: apps/v1.
  • Both call sites are fixed. validateTerminationGracePeriod is included in exactly two templates (daemonset.yaml, dev-pod.yaml) and both moved to }}, so no third site is left trimming.
  • The new suite asserts the real failure mode rather than the symptom: isAPIVersion plus an explicit equal on the apiVersion path, across both the DaemonSet default path and the dev-pod path. Worth noting the PR's own point that helm template and helm lint both exit 0 on the broken render, which is why the existing suite stayed green.
  • Chart version 1.3.0 -> 1.3.1 with appVersion held, which is right for a template-only change.

Full CI is green (71 passing, 0 failing).

@dagil-nvidia

Copy link
Copy Markdown
Collaborator

Approved.

The two open bot threads are stale. Both flag DYN-3741 / NVBug 6555989 at deploy/helm/charts/power-agent/tests/api_version_whitespace_test.yaml:4, but at the current head (cafb4f5) that line reads:

# Regression: right-trim (`-}}`) on silent validateTerminationGracePeriod

No Linear or NVBug reference remains in any of the four files in this PR, so the .ai/linear-ticket-refs.md concern is already addressed.

Both active rulesets set required_review_thread_resolution: true, so those two threads are the only thing holding the merge. Everything else is clear: 71 checks passing with none failing, all four required contexts green (copyright-checks, codeowners, pre-merge-status-check, dynamo-status-check), and the codeowner approval is recorded on behalf of dynamo-operator-codeowners.

Resolving the two threads unblocks it.

For 1.4.0 tracking: this is the main-side fix for NVBug 6555989 (P1). The cherry-pick is #12652, which is green apart from the lychee failure that #12618 fixes.

@kaim-eng
kaim-eng merged commit acda449 into main Aug 4, 2026
112 checks passed
@kaim-eng
kaim-eng deleted the fix/power-agent-helm-apiVersion-whitespace branch August 4, 2026 23:52
pvijayakrish pushed a commit that referenced this pull request Aug 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment::k8s Relates to dynamo deployment in kubernetes fix size/M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants