fix: helm chart is rendering invalid fields in some situations #38947
Conversation
wyattwalter
requested review from
sharat87 and
pratapaprasanna
as code owners
WalkthroughThis pull request introduces updates to Helm chart configurations across multiple template files. The changes include a version increment in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deploy/helm/templates/deployment.yaml (1)
Line range hint
66-82: Consider adding resource limits and configurable timeouts.While the init containers are correctly implemented, consider the following improvements:
- Add resource limits to prevent potential resource exhaustion
- Make health check timeouts configurable via values
- Consider parameterizing the hardcoded image versions
Example addition to values.yaml:
initContainer: redis: resources: limits: memory: "128Mi" cpu: "100m" timeoutSeconds: 300 mongodb: resources: limits: memory: "128Mi" cpu: "100m" timeoutSeconds: 300 postgresql: resources: limits: memory: "128Mi" cpu: "100m" timeoutSeconds: 300Also applies to: 83-93, 94-105
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deploy/helm/Chart.yaml(1 hunks)deploy/helm/templates/deployment.yaml(1 hunks)deploy/helm/templates/persistentVolume.yaml(0 hunks)deploy/helm/templates/service.yaml(0 hunks)
💤 Files with no reviewable changes (2)
- deploy/helm/templates/service.yaml
- deploy/helm/templates/persistentVolume.yaml
✅ Files skipped from review due to trivial changes (1)
- deploy/helm/Chart.yaml
🔇 Additional comments (1)
deploy/helm/templates/deployment.yaml (1)
62-64: LGTM! Fix for empty initContainers issue.The conditional block correctly prevents the inclusion of an empty
initContainerssection when all databases are disabled.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
This PR has been closed because of inactivity. |
Description
Fixing a few situations where the Helm chart can output manifests with invalid manifests. It seems that
helm installis fine with them and they're being corrected on the server side. But some clients like Terraform'skubernetes_manifestis seeing diffs from the local version vs what's on the server.deployment.yaml:
initContainers:linepersistentVolume.yaml:
spec.csijust has a straynfs:🤷 That doesn't conform to the spec https://kubernetes.io/docs/reference/kubernetes-api/config-and-storage-resources/persistent-volume-v1/service.yaml:
nodePort: nullwhich is wiped server-side when applying.Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Warning
Tests have not run on the HEAD d0d2bfb yet
Fri, 31 Jan 2025 21:51:51 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Chores