-
Notifications
You must be signed in to change notification settings - Fork 4
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
meta: extract and improve upload symbol script #38
meta: extract and improve upload symbol script #38
Conversation
@@ -264,7 +266,7 @@ | |||
); | |||
runOnlyForDeploymentPostprocessing = 0; | |||
shellPath = /bin/sh; | |||
shellScript = "if which sentry-cli >/dev/null; then\n if [ -f .env ] && grep -q \"^SENTRY_ORG=\" .env && grep -q \"^SENTRY_PROJECT=\" .env; then\n export $(grep -v '^#' .env | sed '/^\\s*$/d' | xargs)\n else\n echo \"[ERROR] .env does not exist or does not have SENTRY_ORG and SENTRY_PROJECT defined\"\n exit 1\n fi\n if [ ! -n \"$SENTRY_AUTH_TOKEN\" ]; then\n if [ -f ~/.sentryclirc ]; then\n export SENTRY_AUTH_TOKEN=$(grep -oE \"token=([^\\n\\r]*)$\" ~/.sentryclirc | cut -d'=' -f2)\n else \n echo \".sentryclirc does not exist.\"\n fi\n if [ -f ~/.zshrc ] && grep -q \"export SENTRY_AUTH_TOKEN\" ~/.zshrc; then\n grep -m 1 \"export SENTRY_AUTH_TOKEN\" ~/.zshrc > /tmp/ios.sentry-build.tmp && source /tmp/ios.sentry-build.tmp && rm /tmp/ios.sentry-build.tmp\n else\n echo \".zshrc does not exist or does not have SENTRY_AUTH_TOKEN in it.\"\n fi\n fi\n if [ ! -n \"$SENTRY_AUTH_TOKEN\" ]; then\n echo \"[ERROR] must provide SENTRY_AUTH_TOKEN either through command line, .zshrc or .sentryclirc\"\n exit 1\n fi\n\n ERROR=$(sentry-cli upload-dif --include-sources \"$DWARF_DSYM_FOLDER_PATH\" 2>&1 >/dev/null)\n if [ ! $? -eq 0 ]; then\n echo \"warning: sentry-cli - $ERROR\"\n fi\nelse\n echo \"[ERROR] sentry-cli not installed, download from https://github.com/getsentry/sentry-cli/releases\"\n exit 1\nfi\n"; |
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.
If this ever changed, it's also much harder to read a diff if it's written here vs in a separate file, like below
if [ -f ~/.sentryclirc ]; then | ||
export SENTRY_AUTH_TOKEN=$(grep -oE "token=([^\n\r]*)$" ~/.sentryclirc | cut -d'=' -f2) | ||
echo "Using SENTRY_AUTH_TOKEN from .sentryclirc." | ||
fi | ||
if [ -f ~/.zshrc ] && grep -q "export SENTRY_AUTH_TOKEN" ~/.zshrc; then | ||
grep -m 1 "export SENTRY_AUTH_TOKEN" ~/.zshrc > /tmp/ios.sentry-build.tmp && source /tmp/ios.sentry-build.tmp && rm /tmp/ios.sentry-build.tmp | ||
echo "Using SENTRY_AUTH_TOKEN from .zshrc." | ||
fi |
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.
Removed the else
clauses to log where it's not found. I found it more obvious to follow along when it tells me which one it's actually using.
exit 1 | ||
fi | ||
|
||
ERROR=$(sentry-cli upload-dif --force-foreground --include-sources "$DWARF_DSYM_FOLDER_PATH" 2>&1 >/dev/null) |
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.
added --force-foreground
so that the build will fail if symbol upload fails.
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.
🙏
The reason I did this is originally because of the annoying modal popup when symbol upload failed async. But editing the script is not nice in the xcode gui, so i also moved it into a separate file.
Does two things:
a. run
sentry-cli
synchronously. if it fails, it's not worth proceeding with the build because you'll be using the app and sending events that can't be symbolicated. this also avoid the annoying modal popup that appeared whenever it failed asynchronously:b. instead of logging where the auth token isn't found, log where it is found and being used from; this made it a little more obvious to me where it was coming from