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

Failure to locate sentry-cli should result in error, not warning #4564

Closed
armcknight opened this issue Dec 29, 2021 · 7 comments
Closed

Failure to locate sentry-cli should result in error, not warning #4564

armcknight opened this issue Dec 29, 2021 · 7 comments

Comments

@armcknight
Copy link
Member

If a customer has already integrated the invocation of sentry-cli into their Xcode project, we should break their build instead of simply emitting a warning if the executable cannot be located/run (as described here). If we allow their build to succeed, then they will wind up with unsymbolicated results in their dashboard, which is not desirable. In a worse case, they may run a deploy to beta/production on a remote machine where sentry-cli has not been installed, and not know that they are going to encounter these unsymbolicatable results until it's too late. I would want that build to fail if it were me.

A possible additional option that I've used in the past is to allow certain warnings/errors to be configured to be escalated/downgraded with an option like --warnings-as-errors or --errors-as-warnings.

@armcknight
Copy link
Member Author

@getsentry/team-mobile

@philipphofmann
Copy link
Member

I think this should be solved in the bash script for Xcode in our docs: https://docs.sentry.io/platforms/apple/dsym/#uploading-symbols-with-sentry-cli:

if which sentry-cli >/dev/null; then
export SENTRY_ORG=example-org
export SENTRY_PROJECT=example-project
export SENTRY_AUTH_TOKEN=YOUR_AUTH_TOKEN
ERROR=$(sentry-cli upload-dif "$DWARF_DSYM_FOLDER_PATH" 2>&1 >/dev/null)
if [ ! $? -eq 0 ]; then
echo "warning: sentry-cli - $ERROR"
fi
else
echo "warning: sentry-cli not installed, download from https://github.com/getsentry/sentry-cli/releases"
fi

If so, I think we should move this issue to the docs.

@armcknight
Copy link
Member Author

SGTM! Then if it’s ever decided to take on getsentry/sentry-cli#1092, we can revisit other flags/options to provide as mentioned in the description.

@philipphofmann philipphofmann transferred this issue from getsentry/sentry-cli Jan 10, 2022
@imatwawana imatwawana added the Team: Mobile Platform team-mobile label Apr 14, 2022
@philipphofmann philipphofmann self-assigned this Apr 20, 2022
@philipphofmann
Copy link
Member

Hey @armcknight, getsentry/sentry-cli#1092 is completed now. Do you want to revisit this issue now?

@armcknight
Copy link
Member Author

So it sounds like we can leave the onus on the consumer for whether to emit a warning or error, depending on their preferences. We could just include that tidbit in the docs like you said, maybe providing an alternate example.

Did you have anything else in mind for that @philipphofmann ?

@philipphofmann
Copy link
Member

No, this makes sense. Are you up for a PR maybe @armcknight?

@armcknight
Copy link
Member Author

@philipphofmann absolutely, here's the draft: #5147

@kahest kahest closed this as completed Nov 29, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Dec 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Archived in project
Development

No branches or pull requests

4 participants