fix: updates to make scripts region agnostic#1385
Conversation
There was a problem hiding this comment.
Code Review
This pull request improves environment setup scripts by adding validation for critical variables (PROJECT_ID and REGION) and prompting users to enter a region if it is not set. However, several regressions were introduced where double quotes around the REGION variable were removed, which could lead to word splitting or argument shifting issues. Additionally, using a return statement in setenv.sh when the script is executed directly rather than sourced can cause shell errors; a check using BASH_SOURCE should be implemented to handle both execution and sourcing cleanly.
| if [ -z "${PROJECT_ID:-}" ] || [ -z "${REGION:-}" ]; then | ||
| echo "⚠️ Warning: .env was loaded, but is missing PROJECT_ID or REGION." | ||
| echo " Please re-run scripts/setup_lab.sh to configure your environment." | ||
| return 1 2>/dev/null || exit 1 | ||
| fi |
There was a problem hiding this comment.
Using return 1 2>/dev/null || exit 1 in a script that can be executed directly (rather than sourced) will cause Bash to print an error message to standard error: return: can only return from a function or sourced script. To prevent this and handle both execution and sourcing cleanly, you can check if the script is being sourced using BASH_SOURCE.
| if [ -z "${PROJECT_ID:-}" ] || [ -z "${REGION:-}" ]; then | |
| echo "⚠️ Warning: .env was loaded, but is missing PROJECT_ID or REGION." | |
| echo " Please re-run scripts/setup_lab.sh to configure your environment." | |
| return 1 2>/dev/null || exit 1 | |
| fi | |
| if [ -z "${PROJECT_ID:-}" ] || [ -z "${REGION:-}" ]; then | |
| echo "⚠️ Warning: .env was loaded, but is missing PROJECT_ID or REGION." | |
| echo " Please re-run scripts/setup_lab.sh to configure your environment." | |
| if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then | |
| exit 1 | |
| else | |
| return 1 | |
| fi | |
| fi |
| # --------------------------------------------------------------- | ||
| echo "[2/8] Creating BigQuery dataset 'lost_cargo_dataset'..." | ||
| bq --location="$REGION" mk --dataset "$PROJECT_ID:lost_cargo_dataset" 2>/dev/null || true | ||
| bq --location=$REGION mk --dataset "$PROJECT_ID:lost_cargo_dataset" 2>/dev/null || true |
There was a problem hiding this comment.
Removing the double quotes around $REGION is a regression. If REGION is empty or contains spaces, it can cause word splitting or argument shifting issues (e.g., --location would consume mk as its argument, leading to a confusing syntax error). It is best practice to always quote variable expansions.
| bq --location=$REGION mk --dataset "$PROJECT_ID:lost_cargo_dataset" 2>/dev/null || true | |
| bq --location="$REGION" mk --dataset "$PROJECT_ID:lost_cargo_dataset" 2>/dev/null || true |
| SA_EMAIL=$(bq show --format=prettyjson --connection ${REGION}.lost_cargo_conn \ | ||
| | grep "serviceAccountId" | cut -d '"' -f 4) |
There was a problem hiding this comment.
The connection string parameter should be double-quoted to prevent word splitting and globbing, ensuring robust execution even if the region name contains unexpected characters or is empty. Additionally, parsing the JSON output using standard tools like grep and cut is preferred over jq to ensure compatibility across execution environments.
| SA_EMAIL=$(bq show --format=prettyjson --connection ${REGION}.lost_cargo_conn \ | |
| | grep "serviceAccountId" | cut -d '"' -f 4) | |
| SA_EMAIL=$(bq show --format=prettyjson --connection "${REGION}.lost_cargo_conn" \ | |
| | grep "serviceAccountId" | cut -d '"' -f 4) |
References
- In shell scripts, prefer using standard tools like
grepandcutfor parsing command output over tools likejqif the latter's availability or functionality in the execution environment is not guaranteed.
|
|
||
| # Grant the connection's service account access to AlloyDB | ||
| SA_EMAIL_ALLOYDB=$(bq show --format=prettyjson --connection "$REGION.lost_cargo_alloydb_conn" | grep "serviceAccountId" | cut -d '"' -f 4) | ||
| SA_EMAIL_ALLOYDB=$(bq show --format=prettyjson --connection ${REGION}.lost_cargo_alloydb_conn | grep "serviceAccountId" | cut -d '"' -f 4) |
There was a problem hiding this comment.
Removing the double quotes around ${REGION}.lost_cargo_alloydb_conn is a regression. Please keep the double quotes to prevent word splitting and globbing. Also, using standard tools like grep and cut to parse the JSON output is preferred over jq to ensure compatibility.
| SA_EMAIL_ALLOYDB=$(bq show --format=prettyjson --connection ${REGION}.lost_cargo_alloydb_conn | grep "serviceAccountId" | cut -d '"' -f 4) | |
| SA_EMAIL_ALLOYDB=$(bq show --format=prettyjson --connection "${REGION}.lost_cargo_alloydb_conn" | grep "serviceAccountId" | cut -d '"' -f 4) |
References
- In shell scripts, prefer using standard tools like
grepandcutfor parsing command output over tools likejqif the latter's availability or functionality in the execution environment is not guaranteed.
| else | ||
| echo " Creating bucket $BUCKET..." | ||
| gcloud storage buckets create "$BUCKET" --location="$REGION" | ||
| gcloud storage buckets create "$BUCKET" --location=$REGION |
There was a problem hiding this comment.
No description provided.