Fix query service env vars handling for additionalEnvs (closes #578) #591
Conversation
WalkthroughThe pull request modifies the SigNoz chart configuration to enhance environment variable handling for the query service. The changes introduce a more flexible approach to defining environment variables in the Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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
CodeRabbit Configuration File (
|
![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 (2)
charts/signoz/templates/query-service/statefulset.yaml (1)
143-152: LGTM! Well-structured environment variable handling.The implementation elegantly supports both backward-compatible simple values and advanced configurations using Kubernetes native patterns. This allows users to reference values from secrets and configmaps while maintaining compatibility with existing configurations.
Consider adding validation to ensure that when using the map structure, at least one of
valueorvalueFromis specified. Also, adding comments to explain the supported formats would improve maintainability.{{- range $key, $val := .Values.queryService.additionalEnvs }} - name: {{ $key }} + {{- /* Support both simple values and advanced configurations with secrets/configmaps */ -}} {{- if kindIs "map" $val }} + {{- /* Ensure either value or valueFrom is specified */ -}} + {{- if and (not $val.value) (not $val.valueFrom) }} + {{- fail (printf "Environment variable '%s' must specify either 'value' or 'valueFrom'" $key) }} + {{- end }} {{- if $val.value}} value: {{ $val.value | quote }} {{- end}}charts/signoz/values.yaml (1)
599-614: Documentation looks good, consider adding more examples.The documentation clearly explains both configuration methods with good examples. The recommendation to use the flexible structure is helpful for users.
Consider enhancing the documentation with:
- More examples showing different
valueFromsources:additionalEnvs: # Example with ConfigMap CONFIG_KEY: valueFrom: configMapKeyRef: name: my-config key: my-key # Example with Secret API_KEY: valueFrom: secretKeyRef: name: my-secret key: api-key # Example with Field Ref POD_NAME: valueFrom: fieldRef: fieldPath: metadata.name- A note about validation requirements when using the flexible structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/signoz/templates/query-service/statefulset.yaml(1 hunks)charts/signoz/values.yaml(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
charts/signoz/templates/_helpers.tpl (1)
750-768: Consider collisions caused by uppercase conversion.
Converting keys to uppercase and ignoring duplicates may inadvertently drop keys if their uppercase variants match. It's generally advisable to fail fast or warn if collisions occur, rather than silently skipping one. You could add a check to log or surface a warning about potential collisions.- {{- if not (hasKey $processedKeys $key) }} + {{- if hasKey $processedKeys $key }} + {{- /* Optionally log or warn about duplicate uppercase environment variable names */ -}} + {{- else }} {{- $processedKeys = merge $processedKeys (dict $key true) -}} ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/signoz/templates/_helpers.tpl(1 hunks)charts/signoz/templates/query-service/statefulset.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/signoz/templates/query-service/statefulset.yaml
🔇 Additional comments (3)
charts/signoz/templates/_helpers.tpl (3)
743-746: Documentation is well-structured and clear.
Great job on adding a concise explanatory comment above the function definition, making it immediately clear what this template function does.
747-749: Good use of local variables to manage input and processed keys.
The usage of$dictto hold the initial data and$processedKeysto track duplicates is helpful for maintaining readability.
757-765: Validate special types (secrets, configMaps) if needed.
If you plan on supporting additional structures (e.g., further nested objects for secret references), you may need specialized handling logic here. This is a good start, but keep in mind future expansions for more complex environment variable structures.
|
@TheShubhendra, this looks good, can you please bump the SigNoz version in |
Done! |
This PR addresses the issue where the handling of additional environment variables (additionalEnvs) in the query service was not flexible enough. The changes allow for both backward-compatible key-value pairs and more advanced configurations, including value references from secrets and config maps.
Changes made:
Closes #578.
Summary by CodeRabbit
Documentation
Improvements
Version Update