-
Notifications
You must be signed in to change notification settings - Fork 292
ROB-2062 disable sentry for selfhost environment, allow user override #1991
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Signed-off-by: Roi Glinik <[email protected]>
WalkthroughAdds two Helm template helpers to select a Sentry DSN based on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (2)
✏️ Tip: You can disable this entire section by setting 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 |
|
✅ Docker image ready for
Use this tag to pull the image for testing. 📋 Copy commandsgcloud auth configure-docker us-central1-docker.pkg.dev
docker pull us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:b521f9a
docker tag us-central1-docker.pkg.dev/robusta-development/temporary-builds/robusta-runner:b521f9a me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:b521f9a
docker push me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:b521f9aPatch Helm values in one line: helm upgrade --install robusta robusta/robusta \
--reuse-values \
--set runner.image=me-west1-docker.pkg.dev/robusta-development/development/robusta-runner-dev:b521f9a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
helm/robusta/templates/runner.yaml (1)
321-332: Potential issue: Conditional check may not work as intended with literal"".The helper
robusta.sentryDsnreturns""(a literal two-character string with quotes) for non-SaaS environments. The condition{{- if $sentryDsn }}will evaluate this as truthy since it's not an empty string—it's the string"". This means the Secret will still be created withSENTRY_DSN: ""for self-hosted environments, which may not be the intended behavior.Consider updating the helper to return an actually empty string instead of
"", or adjust the condition here:Proposed fix in _helpers.tpl (preferred)
{{- define "robusta.sentryDsn" -}} {{- if .Values.runner.sentry_dsn -}} {{ .Values.runner.sentry_dsn }} {{- else if eq (include "robusta.isSaasEnvironment" .) "true" -}} https://[email protected]/6402199 -{{- else -}} -"" {{- end -}} {{- end -}}
🤖 Fix all issues with AI agents
In `@helm/robusta/templates/_helpers.tpl`:
- Around line 136-144: The helper define "robusta.sentryDsn" returns a literal
quoted string `""` instead of an actual empty value, causing downstream
conditionals to treat it as truthy; update the "robusta.sentryDsn" template to
return an empty string (remove the surrounding quotes) in the final else branch
so the helper yields an actual empty value when Sentry is disabled.
🧹 Nitpick comments (1)
helm/robusta/templates/runner.yaml (1)
333-333: Remove trailing blank line.Static analysis (YAMLlint) flagged an extra blank line at the end of the file.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
helm/robusta/templates/_helpers.tplhelm/robusta/templates/runner.yamlhelm/robusta/values.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
helm/robusta/templates/runner.yaml
[warning] 333-333: too many blank lines (1 > 0)
(empty-lines)
⏰ 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). (4)
- GitHub Check: run_tests
- GitHub Check: build
- GitHub Check: run_tests
- GitHub Check: Deploy docs
🔇 Additional comments (2)
helm/robusta/values.yaml (1)
704-704: LGTM!Removing the hardcoded default DSN and delegating to the helper template is the correct approach for supporting environment-aware Sentry configuration. Self-hosted users will now have Sentry disabled by default, while SaaS environments will continue to use the DSN defined in the helper.
helm/robusta/templates/_helpers.tpl (1)
118-130: LGTM!The
isSaasEnvironmenthelper correctly determines the environment by checking ifROBUSTA_UI_DOMAINis unset or points to a Robusta-managed domain. The iteration overadditional_env_varsis appropriate for finding the environment variable.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Signed-off-by: Roi Glinik <[email protected]>
Signed-off-by: Roi Glinik <[email protected]>
No description provided.