-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix(dev): Do not export SENTRY_DSN in .envrc #35440
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
We started exporting SENTRY_DSN back in #32524 in order to fix errors in lib.sh not being reported. Having this variable exported can cause a lot of errors from a developers' development to show up as dev env errors. Exporting inside of lib.sh does not persist in the developers' environment.
@@ -142,8 +143,6 @@ fi | |||
if [ -n "${SENTRY_DEVENV_NO_REPORT+x}" ]; then | |||
debug "No development environment errors will be reported (since you've defined SENTRY_DEVENV_NO_REPORT)." | |||
else | |||
# This is necessary for the bash-hook in lib.sh to work | |||
export SENTRY_DSN="https://[email protected]/5723503" |
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.
When exporting via direnv
, any Sentry error reporting in the code will pick up the reserved variable.
@@ -16,13 +16,8 @@ if sys.version_info.major < 3: | |||
try: | |||
import sentry_sdk | |||
|
|||
if os.environ.get("SENTRY_DSN"): | |||
sentry_sdk.init(dsn=os.environ["SENTRY_DSN"]) |
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.
I guess this block was redundant since init would have loaded automatically from the reserved variable.
@@ -28,10 +28,13 @@ configure-sentry-cli() { | |||
# We can remove this after it's fixed | |||
# https://github.com/getsentry/sentry-cli/pull/1059 | |||
export SENTRY_CLI_NO_EXIT_TRAP=${SENTRY_CLI_NO_EXIT_TRAP-0} | |||
if [ -n "${SENTRY_DSN+x}" ] && [ -z "${SENTRY_DEVENV_NO_REPORT+x}" ]; then | |||
if [ -z "${SENTRY_DEVENV_NO_REPORT+x}" ]; then |
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.
Since we now don't export the variable.
if ! require sentry-cli; then | ||
curl -sL https://sentry.io/get-cli/ | SENTRY_CLI_VERSION=2.0.4 bash | ||
fi | ||
# This exported variable does not persist outside of the calling script, thus, not affecting other | ||
# parts of the system | ||
export SENTRY_DSN="https://[email protected]/5723503" |
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.
Once the script that calls configure-sentry-cli ends, the variable is lost and the potential impact contained.
@asottile-sentry @Zylphrex would you mind testing that direnv works for you with this PR? (since you two were affected on #33826) |
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.
We started exporting SENTRY_DSN back in #32524 in order to fix errors in lib.sh not being reported. Having this variable exported can cause a lot of errors from a developers' development to show up as dev env errors. Exporting inside of lib.sh does not persist in the developers' environment.
We started exporting SENTRY_DSN back in #32524 in order to fix errors in lib.sh not being reported.
Having this variable exported can cause a lot of errors from a developers' development to show up as dev env errors.
Exporting inside of lib.sh does not persist in the developers' environment.