Skip to content
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

Fix Sentry DSN #1658

Closed
wants to merge 4 commits into from
Closed

Conversation

DanielMcAssey
Copy link
Contributor

Actually fix #1655

This supports having individual DSNs

@@ -353,7 +353,7 @@ services:
- MAX_BRIDGE_PARTICIPANTS
- OCTO_BRIDGE_SELECTION_STRATEGY
- PROSODY_VISITORS_MUC_PREFIX
- SENTRY_DSN="${JICOFO_SENTRY_DSN:-0}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removes support for setting individual DSN values.

SENTRY_DSN is the name the environment variable the sentry library enforces to use.

Copy link
Contributor Author

@DanielMcAssey DanielMcAssey Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable is read during templating, please see the logging.properties

But yes I will have to look at remapping

@@ -1,7 +1,9 @@
{{ if .Env.SENTRY_DSN | default "0" | toBool }}
{{ $SENTRY_DSN := .Env.SENTRY_DSN | default .Env.JICOFO_SENTRY_DSN -}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not help. JICOFO_SENTRY_DSN is not evaluated (as only SENTRY_DSN is)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to just strip the | default "0" | toBool-part.

This is at least working in my environment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think that's best, the problem is remapping, making sure that the value passed by docker-compose is unset

@DanielMcAssey DanielMcAssey marked this pull request as draft December 1, 2023 14:26
@krombel
Copy link
Contributor

krombel commented Dec 1, 2023

As you just changed this PR to beeing a draft I PRd my changes I implemented and tested in my env.

I hope you are fine with it. For reference: #1659

@DanielMcAssey DanielMcAssey deleted the pr/fix-sentry-dsn branch December 1, 2023 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SENTRY_DSN can not be defined
2 participants