fix(otel): OPENFGA_TRACE_OTLP_TLS_ENABLED boolean flag quote#280
fix(otel): OPENFGA_TRACE_OTLP_TLS_ENABLED boolean flag quote#280yannchabed wants to merge 2 commits intoopenfga:mainfrom
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe OpenFGA Helm chart deployment template was updated to change the OPENFGA_TRACE_OTLP_TLS_ENABLED environment variable from an unquoted YAML value to a quoted string literal, ensuring proper YAML formatting. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
cc @rhamzeh as a regular contributor WDYT? |
There was a problem hiding this comment.
Pull request overview
Updates the OpenFGA Helm chart deployment template so OPENFGA_TRACE_OTLP_TLS_ENABLED is rendered as a Kubernetes-compatible string value when enabled, avoiding boolean-type env var rendering.
Changes:
- Quote the rendered value for
OPENFGA_TRACE_OTLP_TLS_ENABLEDindeployment.yamlso it becomes"true"instead oftrue. - Keep the env var conditional on
.Values.telemetry.trace.otlp.tls.enabled.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- if .Values.telemetry.trace.otlp.tls.enabled }} | ||
| - name: OPENFGA_TRACE_OTLP_TLS_ENABLED | ||
| value: {{ .Values.telemetry.trace.otlp.tls.enabled }} | ||
| value: "{{ .Values.telemetry.trace.otlp.tls.enabled }}" | ||
| {{- end }} |
There was a problem hiding this comment.
The CI chart-testing install uses default values (where telemetry.trace.otlp.tls.enabled is false), so this rendering path likely isn’t exercised in tests. Consider adding a charts/openfga/ci/ values file (used by chart-testing) that sets telemetry.trace.otlp.tls.enabled=true to prevent regressions and ensure the env var renders as a string.
| {{- if .Values.telemetry.trace.otlp.tls.enabled }} | ||
| - name: OPENFGA_TRACE_OTLP_TLS_ENABLED | ||
| value: {{ .Values.telemetry.trace.otlp.tls.enabled }} | ||
| value: "{{ .Values.telemetry.trace.otlp.tls.enabled }}" | ||
| {{- end }} |
There was a problem hiding this comment.
The PR description says this change "includes" OPENFGA_TRACE_OTLP_TLS_ENABLED in the deployment, but this template block already existed; the actual change is quoting/casting the value. It may be worth updating the PR description to avoid confusion for reviewers and release notes.
This PR ensures that the
OPENFGA_TRACE_OTLP_TLS_ENABLEDenvironment variable is correctly handled as a boolean string within the Helm chart templates.Description
What problem is being solved?
Updates the OpenFGA Helm chart deployment template so OPENFGA_TRACE_OTLP_TLS_ENABLED is rendered as a Kubernetes-compatible string value when enabled, avoiding boolean-type env var rendering.
How is it being solved?
The issue is resolved by updating the deployment template to ensure that the value of
OPENFGA_TRACE_OTLP_TLS_ENABLEDis explicitly converted to a string (quoted) before being injected into the container environment. This ensures compatibility with Kubernetes API expectations and OpenFGA's configuration logic.What changes are made to solve it?
Quote the rendered value for
OPENFGA_TRACE_OTLP_TLS_ENABLEDin deployment.yaml so it becomes "true" instead of true.Keep the env var conditional on .Values.telemetry.trace.otlp.tls.enabled.
In the current Helm chart, the
OPENFGA_TRACE_OTLP_TLS_ENABLEDvariable—used to toggle TLS for OpenTelemetry Protocol (OTLP) tracing—was not being correctly processed. In Kubernetes manifests, environment variable values must be strings. If a boolean (true/false) is passed directly from Helm'svalues.yamlwithout explicit casting or quoting, it causes template rendering errors or be misinterpreted by the OpenFGA server at runtime.References
Review Checklist
I have added documentation for new/changed functionality in this PR or in a PR to [openfga.dev](https://github.com/openfga/openfga.dev)mainI have added tests to validate that the change in functionality is working as expectedNo test coverageNext Step: Would you like me to help you draft a specific comment for the maintainers or analyze the related CI/CD failures if there are any?
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.