Skip to content

Conversation

@tmonty12
Copy link
Contributor

@tmonty12 tmonty12 commented Oct 29, 2025

Overview:

The etcd pre upgrade job is still running on helm installation because the field is preUpgradeJob not preUpgrade. This PR fixes that

https://github.com/bitnami/charts/tree/main/bitnami/etcd

Details:

Where should the reviewer start?

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • closes GitHub issue: #xxx

Summary by CodeRabbit

  • Chores
    • Renamed etcd pre-upgrade configuration key in Helm deployment values. Users deploying via Helm should update their configuration files to reflect the new key name.

@tmonty12 tmonty12 requested a review from a team as a code owner October 29, 2025 21:51
@github-actions github-actions bot added the fix label Oct 29, 2025
@tmonty12 tmonty12 enabled auto-merge (squash) October 29, 2025 21:54
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 2025

Walkthrough

A configuration key in the Helm chart values file is renamed from etcd.preUpgrade to etcd.preUpgradeJob, updating the control-flow hook identifier for pre-upgrade validation jobs while preserving the inner enabled setting structure.

Changes

Cohort / File(s) Change Summary
Helm Platform Configuration
deploy/cloud/helm/platform/values.yaml
Renamed etcd configuration key from preUpgrade to preUpgradeJob, maintaining the enabled semantics for pre-upgrade job validation

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify all references to the old key name are updated across the codebase and documentation
  • Confirm backward compatibility considerations or migration guidance if needed

Poem

🐰 A key was renamed with care and grace,
From preUpgrade to preUpgradeJob's place,
In Helm's gentle hands, the config now flows,
With a hop and a skip, the validation goes! ✨

Pre-merge checks

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description follows the template structure but is incomplete. The Overview section is well-filled with clear context explaining the issue and includes a reference to the Bitnami chart conventions. However, two required sections are entirely empty with only template comment placeholders remaining: the Details section lacks description of the actual changes made, and the "Where should the reviewer start" section provides no file-level guidance. Additionally, the Related Issues section contains a placeholder issue number (#xxx) rather than an actual GitHub issue reference. Complete the PR description by filling in the Details section with a description of the specific file and configuration changes, and add guidance in the "Where should the reviewer start" section (e.g., pointing to deploy/cloud/helm/platform/values.yaml). Replace the placeholder issue number (#xxx) with the actual related GitHub issue number, or remove the Related Issues section if no specific issue is associated with this change.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "fix: platform helm chart value etcd preUpgrade not working" directly corresponds to the main change in the changeset. The raw summary shows the change is renaming the etcd configuration key from preUpgrade to preUpgradeJob in the Helm values file, and the title accurately captures this issue and its fix. The title is concise, clear, and specific enough that a teammate scanning the history would understand the primary change is about correcting an etcd Helm chart configuration value.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7b709c4 and dfa6970.

📒 Files selected for processing (1)
  • deploy/cloud/helm/platform/values.yaml (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#3100
File: deploy/cloud/operator/cmd/main.go:186-190
Timestamp: 2025-09-17T22:35:40.674Z
Learning: The mpiRunSecretName validation in deploy/cloud/operator/cmd/main.go is safe for upgrades because the Helm chart automatically populates dynamo-operator.dynamo.mpiRun.secretName with a default value of "mpi-run-ssh-secret" and includes SSH key generation functionality via sshKeygen.enabled: true.
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#3100
File: deploy/cloud/operator/cmd/main.go:186-190
Timestamp: 2025-09-17T22:35:40.674Z
Learning: The mpiRunSecretName validation in deploy/cloud/operator/cmd/main.go is safe for Helm-based upgrades because the chart automatically provides default values (secretName: "mpi-run-ssh-secret", sshKeygen.enabled: true) and the deployment template conditionally injects the --mpi-run-ssh-secret-name flag, ensuring existing installations get the required configuration during upgrades.
📚 Learning: 2025-09-17T22:35:40.674Z
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#3100
File: deploy/cloud/operator/cmd/main.go:186-190
Timestamp: 2025-09-17T22:35:40.674Z
Learning: The mpiRunSecretName validation in deploy/cloud/operator/cmd/main.go is safe for Helm-based upgrades because the chart automatically provides default values (secretName: "mpi-run-ssh-secret", sshKeygen.enabled: true) and the deployment template conditionally injects the --mpi-run-ssh-secret-name flag, ensuring existing installations get the required configuration during upgrades.

Applied to files:

  • deploy/cloud/helm/platform/values.yaml
📚 Learning: 2025-09-17T22:35:40.674Z
Learnt from: julienmancuso
PR: ai-dynamo/dynamo#3100
File: deploy/cloud/operator/cmd/main.go:186-190
Timestamp: 2025-09-17T22:35:40.674Z
Learning: The mpiRunSecretName validation in deploy/cloud/operator/cmd/main.go is safe for upgrades because the Helm chart automatically populates dynamo-operator.dynamo.mpiRun.secretName with a default value of "mpi-run-ssh-secret" and includes SSH key generation functionality via sshKeygen.enabled: true.

Applied to files:

  • deploy/cloud/helm/platform/values.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: sglang (amd64)
  • GitHub Check: operator (amd64)
  • GitHub Check: trtllm (amd64)
  • GitHub Check: vllm (amd64)
  • GitHub Check: Build and Test - dynamo
🔇 Additional comments (1)
deploy/cloud/helm/platform/values.yaml (1)

169-172: Correct field name aligns with Bitnami etcd chart conventions; change is complete.

The rename from preUpgrade to preUpgradeJob is correct and verified. No lingering references to the old key exist in the codebase, and the new key is now properly positioned for the Bitnami etcd chart to recognize. The change is isolated to the values configuration with no template modifications required, as etcd is managed as an external Helm dependency.


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.

❤️ Share

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

@tmonty12 tmonty12 force-pushed the tmonty12/correct-etcd-helm-value branch from dfa6970 to 9d5030a Compare October 30, 2025 22:54
@tmonty12 tmonty12 merged commit bfb2574 into main Oct 30, 2025
26 of 34 checks passed
@tmonty12 tmonty12 deleted the tmonty12/correct-etcd-helm-value branch October 30, 2025 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants