-
Notifications
You must be signed in to change notification settings - Fork 83
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: discourage authToken
usage and link to updated docs
#325
fix: discourage authToken
usage and link to updated docs
#325
Conversation
if (sentryHook.config?.authToken) { | ||
WarningAggregator.addWarningAndroid( | ||
'sentry-expo', | ||
'Sentry `authToken` found in app.json. Avoid committing this value to your repository, configure it through `SENTRY_AUTH_TOKEN` environment variable instead. See: https://docs.expo.dev/guides/using-sentry/#app-configuration' | ||
); | ||
WarningAggregator.addWarningIOS( | ||
'sentry-expo', | ||
'Sentry `authToken` found in app.json. Avoid committing this value to your repository, configure it through `SENTRY_AUTH_TOKEN` environment variable instead. See: https://docs.expo.dev/guides/using-sentry/#app-configuration' | ||
); | ||
} |
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.
"authToken"
also needs to be omitted from the missingProperties
array at line 74 so that it doesn't warn the user that the field is missing.
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.
@jamsch The missing properties doesn't warn users when not defined. The only thing it's doing is adding a comment within the generated sentry.properties
file to mention that it's falling back to the SENTRY_AUTH_TOKEN
.
We could modify that comment, but I do think it could be useful when users stumble upon the configuration after running $ npx expo prebuild
.
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've updated the message, and added another comment when using authToken
. Just to cover users ignoring the warning, and opening the sentry.properties
file.
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.
@byCedric Could you double check that? I'm currently forced to set authToken
to false
in the config in order for the WarningAggregator message to disappear.
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.
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.
@jamsch, just released this: 6.1.1
. Thanks for your help :)
50644d9
to
4f3bc60
Compare
Checklist
Why
See #321
How
Added a warning whenever you configure
authToken
through the post-publish hook.Test Plan
$ yarn create expo-app ./test-authtoken
$ cd ./test-authtoken
$ yarn expo install sentry-expo
app.json
with the post-publish token, includingauthToken
$ yarn expo prebuild