Conversation
…e sentry <foo> command Fixes #3526 Previously, if this was executed: ```bash ./scripts/backup.sh --no-report-self-hosted-issues ``` The final backup command that will be executed is: ```bash docker compose run -v "${PWD}/sentry:/sentry-data/backup" --rm -T -e SENTRY_LOG_LEVEL=CRITICAL web export --no-report-self-hosted-issues /sentry-data/backup/backup.json ``` Which is invalid. This PR strips all known flags specific to self-hosted before passing it onto Sentry. One other option is to enable "ignore unknown options" on click (the CLI dependency) on Sentry, but I don't want to go to that route.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3831 +/- ##
=======================================
Coverage 99.45% 99.45%
=======================================
Files 3 3
Lines 183 183
=======================================
Hits 182 182
Misses 1 1 ☔ View full report in Codecov by Sentry. |
| MINIMIZE_DOWNTIME="${MINIMIZE_DOWNTIME:-}" | ||
| REPORT_SELF_HOSTED_ISSUES="${REPORT_SELF_HOSTED_ISSUES:-}" | ||
|
|
||
| while (($#)); do |
There was a problem hiding this comment.
Why not just add --minimize-downtime to the scripts/_lib.sh? That way we don't need to maintain 3 areas of similar logic
There was a problem hiding this comment.
Because it won't be shifted by Bash and would be included on the Docker command. The problem was this:
cmd="backup $1"
source scripts/_lib.sh
$cmdIf you do ./scripts/backup.sh --minimize-downtime, the resulting cmd would be backup --minimize-downtime. If you want to add another known flag for the backup script, like --silent, you would have to do backup --sillent --minimize-downtime. It will be hard to make users aware that they are not supposed to do backup --minimize-downtime --silent.
So if you omit those known flags before passing the command, it will work as expected. This also would open the possibility of doing cmd="backup $@" (I don't know if this correct, but I hope you get the right idea) to allow more than one parameter.
Fixes #3526
Previously, if this was executed:
The final backup command that will be executed is:
Which is invalid. This PR strips all known flags specific to self-hosted before passing it onto Sentry.
One other option is to enable "ignore unknown options" on click (the CLI dependency) on Sentry, but I don't want to go to that route.