Bash events collector is extremely poorly designed

[George3d6]

Important Details

How are you running Sentry?

Saas (sentry.io)

Adding the lines:

curl -sL https://sentry.io/get-cli/ | bash;
export SENTRY_DSN='blahbalh';
eval "$(sentry-cli bash-hook)";

on top of my bash script, just after the #!/bin/bash

Description

Due to set -e or some other modification, sentry looking causes prefectly valid bash scripts to now fail.

For example:

path_to_x="$(which x)"
if [ -z "${path_to_x}" ];
then
    echo "x is installed, installing";
    ... etc;
fi;

This used to be a valid paradigm to find if the user had x installed in his path, now it becomes invalid.

Certain other things such as otherwise valid echo statements also seem to break.

The HORRIBLE thing about this is that sentry seems/is intended for already existing code, so by significantly altering the behavior of the code and causing it to break when it would otherwise work, this makes it essentially impossible to add sentry to an existing codebase.

Even worst, this could cause someone that's uncareful to break important code, since set -e is runtime based and might only lead to weird errors in various branches that are untouched by tests.

Steps to Reproduce

Write a bash script with syntax that normally works but breaks due to e.g .set -e (see example above)

What you expected to happen

I'd expect an error monitoring software not to heavily modify the behavior of the scripts it's included to in a way that might be obscure to the user and will cause issues with existing code.

I'd expect more obvious warning about this in the docs if no workaround exists (i.e. set -e is require to captrue errors).

Note 1: I'm aware there might be a "best practice" to use set -e, also not using bash scripts anywhere is probably a "best practice", and having 100% unittest coverage, and writing everything in rust while at it. But given the nature of software, sentry is likely to be used in exactly the kind of software that doesn't follow best practices, so I think the argument from best practice does't really make sense.

Note 2: Maybe not the correct place for this, but i wanted to signal boost the issue since this seems like a horrible design overstep. I.e. the kind of thing that, if required should be AT THE TOP OF THE DOCS IN BIG RED LETTERS WARNING THE USER

Note 3: Not an issue in my specific case, I think I know some workarounds, but think of someone that has e.g. 500,1000,5000 lines worth of bash. Or maybe some slightly custom bash-bc shell, etc. This would make sentry useless in a best case scenario and it could cause a lot of damage in a worst case scenario.

[BYK]

Hi @George3d6 and thanks for reporting this issue! Happy to come by a passionate sentry-cli <-> bash folk like myself as I want to use this feature over at getsentry/onpremise#740 but it is not as straightforward as you explained.

This part of the CLI is a bit neglected and hoping that this issue will help us prioritize it.

[BYK]

Ping @kamilogorek @jan-auer

[mitsuhiko]

Generelly I’m not sure what good ways are to improve the experience. Bash is very limited in what is possible. Maybe the right thing to do would just be to remove the functionality from sentry-cli or mark it as deprecated at least.

[untitaker]

Can we print out a warning to stderr instructing the user to enable set -e in the script instead of setting it ourselves? It doesn't really solve the usecase of running the SDK in a script that can't use set -e, but the added behavior (printing) is less severe.

[github-actions[bot]]

This issue has gone three weeks without activity. In another week, I will close it.

But! If you comment or otherwise update it, I will reset the clock, and if you label it Status: Backlog or Status: In Progress, I will leave it alone ... forever!

---

"A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀

[raidlman]

Just wanted to comment on this issue, since I had a different problem with the sentry bash-hook script and I agree with you that in some aspects it's lacking functionality. Imho bash error handling should not be included in the hook but should be part of the script that includes the hook.

That being said, you can prevent those commands from failing by changing your call like so:

path_to_x="$(which x || true)"
if [ -z "${path_to_x}" ];
then
    echo "x is installed, installing";
    ... etc;
fi;

Btw, to see if a command is installed, you should avoid using which. Instead, you should use one of the builtin commands. See here.