From 06bc31a5927fbde4057c0565d6c1b3615a19aec8 Mon Sep 17 00:00:00 2001 From: Bruno Bronosky Date: Fri, 11 Sep 2020 23:44:26 -0500 Subject: [PATCH 1/4] Use bash "new test" `[[` to avoid tricky bug See: https://tldp.org/LDP/abs/html/comparison-ops.html#FTN.AEN3669 The old `[` test has a problem checking for empty strings in combination with logical OR/AND. In this case, it causes the error message: ./install.sh: line 271: [: ==: unary operator expected --- install.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/install.sh b/install.sh index b440576f3df..2a5dea1bb74 100755 --- a/install.sh +++ b/install.sh @@ -268,7 +268,7 @@ fi echo "" echo "Setting up database..." -if [ $CI ] || [ $SKIP_USER_PROMPT == 1 ]; then +if [[ -n "$CI" || "$SKIP_USER_PROMPT" == 1 ]]; then $dcr web upgrade --noinput echo "" echo "Did not prompt for user creation due to non-interactive shell." From 5ef801224a937f685fc7529dbb5972d53dd94b20 Mon Sep 17 00:00:00 2001 From: Bruno Bronosky Date: Sat, 12 Sep 2020 00:23:05 -0500 Subject: [PATCH 2/4] Fix bug in `((` arithmetic expansion by using `[[` If the `:` side of the `||` were ever called, this `if` condition would cause the error: syntax error: operand expected (error token is "== 0") --- install.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/install.sh b/install.sh index 2a5dea1bb74..64345ecc513 100755 --- a/install.sh +++ b/install.sh @@ -119,9 +119,9 @@ fi #SSE4.2 required by Clickhouse (https://clickhouse.yandex/docs/en/operations/requirements/) # On KVM, cpuinfo could falsely not report SSE 4.2 support, so skip the check. https://github.com/ClickHouse/ClickHouse/issues/20#issuecomment-226849297 IS_KVM=$(docker run --rm busybox grep -c 'Common KVM processor' /proc/cpuinfo || :) -if (($IS_KVM == 0)); then +if [[ "$IS_KVM" -eq 0 ]]; then SUPPORTS_SSE42=$(docker run --rm busybox grep -c sse4_2 /proc/cpuinfo || :) - if (($SUPPORTS_SSE42 == 0)); then + if [[ "$SUPPORTS_SSE42" -eq 0 ]]; then echo "FAIL: The CPU your machine is running on does not support the SSE 4.2 instruction set, which is required for one of the services Sentry uses (Clickhouse). See https://git.io/JvLDt for more info." exit 1 fi From 2c8916ed1126fedf85b6c95047cf56b9275af46a Mon Sep 17 00:00:00 2001 From: Bruno Bronosky Date: Sat, 12 Sep 2020 00:32:00 -0500 Subject: [PATCH 3/4] Make bash `if` tests more consistent - Change remaining uses of `[` to `[[` - Don't use quotes around numbers used in arithmetic tests - Use quotes around variables and command substitutions that could be null --- install.sh | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/install.sh b/install.sh index 64345ecc513..22f1772a2d5 100755 --- a/install.sh +++ b/install.sh @@ -60,12 +60,12 @@ trap_with_arg() { DID_CLEAN_UP=0 # the cleanup function will be the exit point cleanup () { - if [ "$DID_CLEAN_UP" -eq 1 ]; then + if [[ "$DID_CLEAN_UP" -eq 1 ]]; then return 0; fi DID_CLEAN_UP=1 - if [ "$1" != "EXIT" ]; then + if [[ "$1" != "EXIT" ]]; then echo "An error occurred, caught SIG$1 on line $2"; if [[ "$MINIMIZE_DOWNTIME" ]]; then @@ -93,7 +93,7 @@ function ver () { echo "$@" | awk -F. '{ printf("%d%03d%03d", $1,$2,$3); }'; } # Thanks to https://stackoverflow.com/a/25123013/90297 for the quick `sed` pattern function ensure_file_from_example { - if [ -f "$1" ]; then + if [[ -f "$1" ]]; then echo "$1 already exists, skipped creation." else echo "Creating $1..." @@ -101,17 +101,17 @@ function ensure_file_from_example { fi } -if [ $(ver $DOCKER_VERSION) -lt $(ver $MIN_DOCKER_VERSION) ]; then +if [[ "$(ver $DOCKER_VERSION)" -lt "$(ver $MIN_DOCKER_VERSION)" ]]; then echo "FAIL: Expected minimum Docker version to be $MIN_DOCKER_VERSION but found $DOCKER_VERSION" exit 1 fi -if [ $(ver $COMPOSE_VERSION) -lt $(ver $MIN_COMPOSE_VERSION) ]; then +if [[ "$(ver $COMPOSE_VERSION)" -lt "$(ver $MIN_COMPOSE_VERSION)" ]]; then echo "FAIL: Expected minimum docker-compose version to be $MIN_COMPOSE_VERSION but found $COMPOSE_VERSION" exit 1 fi -if [ "$RAM_AVAILABLE_IN_DOCKER" -lt "$MIN_RAM" ]; then +if [[ "$RAM_AVAILABLE_IN_DOCKER" -lt "$MIN_RAM" ]]; then echo "FAIL: Expected minimum RAM available to Docker to be $MIN_RAM MB but found $RAM_AVAILABLE_IN_DOCKER MB" exit 1 fi @@ -157,7 +157,7 @@ fi replace_tsdb() { if ( - [ -f "$SENTRY_CONFIG_PY" ] && + [[ -f "$SENTRY_CONFIG_PY" ]] && ! grep -xq 'SENTRY_TSDB = "sentry.tsdb.redissnuba.RedisSnubaTSDB"' "$SENTRY_CONFIG_PY" ); then # Do NOT indent the following string as it would be reflected in the end result, @@ -230,11 +230,11 @@ else fi ZOOKEEPER_SNAPSHOT_FOLDER_EXISTS=$($dcr zookeeper bash -c 'ls 2>/dev/null -Ubad1 -- /var/lib/zookeeper/data/version-2 | wc -l | tr -d '[:space:]'') -if [ "$ZOOKEEPER_SNAPSHOT_FOLDER_EXISTS" -eq "1" ]; then +if [[ "$ZOOKEEPER_SNAPSHOT_FOLDER_EXISTS" -eq 1 ]]; then ZOOKEEPER_LOG_FILE_COUNT=$($dcr zookeeper bash -c 'ls 2>/dev/null -Ubad1 -- /var/lib/zookeeper/log/version-2/* | wc -l | tr -d '[:space:]'') ZOOKEEPER_SNAPSHOT_FILE_COUNT=$($dcr zookeeper bash -c 'ls 2>/dev/null -Ubad1 -- /var/lib/zookeeper/data/version-2/* | wc -l | tr -d '[:space:]'') # This is a workaround for a ZK upgrade bug: https://issues.apache.org/jira/browse/ZOOKEEPER-3056 - if [ "$ZOOKEEPER_LOG_FILE_COUNT" -gt "0" ] && [ "$ZOOKEEPER_SNAPSHOT_FILE_COUNT" -eq "0" ]; then + if [[ "$ZOOKEEPER_LOG_FILE_COUNT" -gt 0 ]] && [[ "$ZOOKEEPER_SNAPSHOT_FILE_COUNT" -eq 0 ]]; then $dcr -v $(pwd)/zookeeper:/temp zookeeper bash -c 'cp /temp/snapshot.0 /var/lib/zookeeper/data/version-2/snapshot.0' $dc run -d -e ZOOKEEPER_SNAPSHOT_TRUST_EMPTY=true zookeeper fi @@ -282,7 +282,7 @@ fi SENTRY_DATA_NEEDS_MIGRATION=$(docker run --rm -v sentry-data:/data alpine ash -c "[ ! -d '/data/files' ] && ls -A1x /data | wc -l || true") -if [ "$SENTRY_DATA_NEEDS_MIGRATION" ]; then +if [[ -n "$SENTRY_DATA_NEEDS_MIGRATION" ]]; then echo "Migrating file storage..." # Use the web (Sentry) image so the file owners are kept as sentry:sentry # The `\"` escape pattern is to make this compatible w/ Git Bash on Windows. See #329. @@ -291,7 +291,7 @@ if [ "$SENTRY_DATA_NEEDS_MIGRATION" ]; then fi -if [ ! -f "$RELAY_CREDENTIALS_JSON" ]; then +if [[ ! -f "$RELAY_CREDENTIALS_JSON" ]]; then echo "" echo "Generating Relay credentials..." From bbba939ed00528aa5062fc6fcb0b07b6c67b76f1 Mon Sep 17 00:00:00 2001 From: Bruno Bronosky Date: Sat, 12 Sep 2020 00:39:10 -0500 Subject: [PATCH 4/4] Use `-n` and `-z` when testing for empty and not --- install.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/install.sh b/install.sh index 22f1772a2d5..e94621bfa5b 100755 --- a/install.sh +++ b/install.sh @@ -68,14 +68,14 @@ cleanup () { if [[ "$1" != "EXIT" ]]; then echo "An error occurred, caught SIG$1 on line $2"; - if [[ "$MINIMIZE_DOWNTIME" ]]; then + if [[ -n "$MINIMIZE_DOWNTIME" ]]; then echo "*NOT* cleaning up, to clean your environment run \"docker-compose stop\"." else echo "Cleaning up..." fi fi - if [[ ! "$MINIMIZE_DOWNTIME" ]]; then + if [[ -z "$MINIMIZE_DOWNTIME" ]]; then $dc stop &> /dev/null fi } @@ -218,7 +218,7 @@ $dc build --force-rm --parallel echo "" echo "Docker images built." -if [[ "$MINIMIZE_DOWNTIME" ]]; then +if [[ -n "$MINIMIZE_DOWNTIME" ]]; then # Stop everything but relay and nginx $dc rm -fsv $($dc config --services | grep -v -E '^(nginx|relay)$') else @@ -246,7 +246,7 @@ $dcr snuba-api migrations migrate --force echo "" # Very naively check whether there's an existing sentry-postgres volume and the PG version in it -if [[ $(docker volume ls -q --filter name=sentry-postgres) && $(docker run --rm -v sentry-postgres:/db busybox cat /db/PG_VERSION 2>/dev/null) == "9.5" ]]; then +if [[ -n "$(docker volume ls -q --filter name=sentry-postgres)" && "$(docker run --rm -v sentry-postgres:/db busybox cat /db/PG_VERSION 2>/dev/null)" == "9.5" ]]; then docker volume rm sentry-postgres-new || true # If this is Postgres 9.5 data, start upgrading it to 9.6 in a new volume docker run --rm \