style: unify Bash code style #5568
Conversation
jaskfla
force-pushed
the
shellharden
branch
2 times, most recently
from
89d8fa2 to
993558a
Compare
| DIR=$(dirname "$0") | ||
|
|
||
|
|
||
| DIR=$(pwd "$0") |
There was a problem hiding this comment.
Immediately overwritten in line 4
jaskfla
changed the title
| echo -e "${RED}Error: $DB_NAME database does not exist!${RESET}" | ||
| echo "To create it, get the .env file from LastPass then run:" | ||
| echo 'To create it, make sure you have the environment variables from Bitwarden and run:' | ||
| echo | ||
| echo -e " ${BOLD}yarn workspace @tupaia/database setup-test-database${RESET}" | ||
| echo | ||
| echo -e "If you’re missing environment variables, see ${MAGENTA}https://beyond-essential.slab.com/posts/tupaia-monorepo-setup-v5egpdpq#hvfnz-set-environment-variables${RESET}." |
jaskfla
force-pushed
the
shellharden
branch
12 times, most recently
from
07069e7 to
d794119
Compare
remove unused var
portable shebang in `mergeEnvForDB.sh`
jaskfla
force-pushed
the
shellharden
branch
3 times, most recently
from
cf05c60 to
1e92207
Compare
| done | ||
|
|
||
| # remove any duplicates | ||
| deduplicated_union=() |
There was a problem hiding this comment.
Never used and not exported
# Conflicts: # scripts/bash/backendStartDev.sh
# Conflicts: # scripts/bash/ansiControlSequences.sh # scripts/bash/getDeployablePackages.sh
# Conflicts: # scripts/bash/buildNonInternalDependencies.sh
WalkthroughThis set of changes standardizes and modernizes shell script practices across numerous scripts in the codebase. The updates include switching shebang lines to use 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
scripts/docker/downloadEnvironmentVariables.sh (1)
1-2: Unsure Potential duplicate file: this script appears to be largely a copy of the canonicaldownloadEnvironmentVariables.sh(see #5516). Consider deleting or consolidating to avoid future divergence.
🧹 Nitpick comments (23)
packages/database/scripts/migrateCreate.sh (1)
1-1: Shebang update improves portability. Unsure: consider addingset -eafter the shebang to enforce immediate exit on errors and maintain consistency with other scripts.scripts/bash/pm2startInline.sh (1)
5-5: Unsure Quote sourcing path for consistency?
You might consider sourcing with a fully quoted path to handle spaces safely, e.g.:. "$(dirname "${BASH_SOURCE[0]}")/ansiControlSequences.sh"packages/devops/scripts/ci/validateTypesAndDbSchemaInSync.sh (1)
8-8: Unsure Use dot (.) instead ofsourcefor consistency?
Most scripts in this PR use.for sourcing. Consider replacingsource "$SCRIPT_DIR/.../mergeEnvForDB.sh"with. "$SCRIPT_DIR/.../mergeEnvForDB.sh"to maintain uniform style.packages/data-api/scripts/dropAnalyticsMaterializedView.sh (1)
11-11: Unsure: Consider quoting the password export for consistency and to prevent word splitting:-export PGPASSWORD=$DB_PASSWORD +export PGPASSWORD="$DB_PASSWORD"packages/data-api/scripts/fastRefreshAnalyticsTable.sh (1)
13-13: Unsure: Consider quoting the password export for consistency:-export PGPASSWORD=$DB_PASSWORD +export PGPASSWORD="$DB_PASSWORD"packages/data-api/scripts/installMvRefreshModule.sh (1)
4-4: Unsure: Other scripts in this PR use.for sourcing; consider replacingsourcewith.to unify style.packages/data-api/scripts/patchMvRefreshModule.sh (3)
9-10: Unsure more idiomatic empty-string check
Consider using[[ -z $COMMAND ]]instead of[[ $COMMAND = '' ]]for clarity.
14-17: Unsure useread -rfor patch names
To prevent backslash-escaping surprises, prefer:read -r PATCH_NAME
31-33: Unsure more idiomatic empty-string check
Same as above,[[ -z $VERSION ]]reads more clearly than[[ $VERSION = '' ]].packages/data-lake-api/scripts/checkTestDataLakeExists.sh (1)
11-11: Unsure quote allpsqlexpansions
Ensure host, port, and user flags are quoted to prevent edge-case breakage:-PGPASSWORD=$DB_PG_PASSWORD psql -p $DATA_LAKE_DB_PORT -X -A -h $DATA_LAKE_DB_URL -U $DB_PG_USER … +PGPASSWORD="$DB_PG_PASSWORD" psql -p "$DATA_LAKE_DB_PORT" -X -A -h "$DATA_LAKE_DB_URL" -U "$DB_PG_USER" …packages/devops/scripts/ci/utils.sh (2)
11-23: Enhanceansi_colorto respect NO_COLOR
Unsure if needed, but consider detectingNO_COLORinansi_colorand returning an empty string when set.
42-47: Use-zfor empty string check
Consider simplifying[[ $branch_name = '' ]]to[[ -z $branch_name ]]for clarity.packages/devops/scripts/ci/validateBranchName.sh (2)
1-5: Use absolute script path resolution
Consider replacingDIR=$(dirname "$0")with something likeDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"for more reliable sourcing.
20-26: Clarify parameter expansion in suffix check
Unsure if parsing works as intended, but using${branch_name}-apiinstead of$branch_name-apimay improve readability in the special-apisuffix condition.scripts/bash/mergeEnvForDB.sh (2)
8-8: Quote variables in conditional tests
Consider quoting"$dir"in[[ $dir != '/' ]], e.g.,[[ "$dir" != "/" ]], to be consistent.
32-38: Quote file paths in existence check
Consider adding quotes around$filein[[ -f $file ]], i.e.,[[ -f "$file" ]], to guard against spaces.packages/devops/scripts/ci/triggerRedeploy.sh (1)
39-45: Optimize JSON parsing without UUOC
You can simplifycat "$RESPONSE_FILE" | jq -r '.[] | @base64'tojq -r '.[] | @base64' "$RESPONSE_FILE"to avoid the extracat.packages/data-api/scripts/uninstallMvRefreshModule.sh (1)
5-5: Use reliable script path resolution
Consider usingDIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"instead of$(pwd "$0")when sourcingmergeEnvForDB.sh.packages/database/scripts/checkTestDatabaseExists.sh (1)
11-13: Unsure Use modern conditional syntax
Prefer[[ ... ]]over[for this test and quote the overall expression, e.g.:if [[ $(PGPASSWORD="$DB_PG_PASSWORD" psql ... ) = 1 ]]; thenThis is more robust and consistent with the style guide.
scripts/bash/buildInternalDependencies.sh (2)
49-53: Unsure Remove unused variablebuild_ts_args
build_ts_argsis assigned but never used. You may want to remove it or integrate it into the build commands.🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 50-50: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 52-52: Argument mixes string and array. Use * or separate argument.
(SC2145)
[warning] 52-52: eval negates the benefit of arrays. Drop eval to preserve whitespace/symbols (or eval as string).
(SC2294)
54-58: Unsure Avoid mixing strings and arrays ineval
SC2145/SC2294 warns about mixing array expansions andeval. Consider building a command array or unwrapping arrays to separate arguments instead ofevalfor better safety.scripts/bash/getInternalDependencies.sh (2)
5-10: Unsure Use idiomatic and quoted string check
Consider simplifying the test and quoting the variable:- if [[ $1 != '' ]]; then + if [[ -n "$1" ]]; thenThis is more idiomatic and safer against empty values.
77-78: Unsure Quote array elements in comparison
Inside[[ ... ]], quote both sides to prevent globbing:- if (( i != j )) && [[ ${internal_dependencies[i]} = ${internal_dependencies[j]} ]]; then + if (( i != j )) && [[ "${internal_dependencies[i]}" = "${internal_dependencies[j]}" ]]; thenThis ensures exact string matching.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 77-77: Quote the right-hand side of = in [[ ]] to prevent glob matching.
(SC2053)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
packages/data-api/scripts/buildAnalyticsMaterializedView.sh(1 hunks)packages/data-api/scripts/dropAnalyticsMaterializedView.sh(1 hunks)packages/data-api/scripts/fastRefreshAnalyticsTable.sh(1 hunks)packages/data-api/scripts/fullRefreshAnalyticsTable.sh(1 hunks)packages/data-api/scripts/installMvRefreshModule.sh(2 hunks)packages/data-api/scripts/patchMvRefreshModule.sh(1 hunks)packages/data-api/scripts/refreshAnalyticsTable.sh(1 hunks)packages/data-api/scripts/uninstallMvRefreshModule.sh(2 hunks)packages/data-lake-api/scripts/checkTestDataLakeExists.sh(1 hunks)packages/data-lake-api/scripts/setupTestDataLake.sh(1 hunks)packages/database/scripts/checkTestDatabaseExists.sh(1 hunks)packages/database/scripts/dumpDatabase.sh(5 hunks)packages/database/scripts/migrateCreate.sh(1 hunks)packages/devops/scripts/ci/triggerRedeploy.sh(3 hunks)packages/devops/scripts/ci/utils.sh(2 hunks)packages/devops/scripts/ci/validateBranchName.sh(3 hunks)packages/devops/scripts/ci/validateNewMigrations.sh(5 hunks)packages/devops/scripts/ci/validateTests.sh(1 hunks)packages/devops/scripts/ci/validateTypesAndDbSchemaInSync.sh(1 hunks)packages/devops/scripts/deployment-aws/buildDeployablePackages.sh(1 hunks)packages/devops/scripts/deployment-aws/checkoutLatest.sh(2 hunks)packages/devops/scripts/deployment-aws/configureNginx.sh(1 hunks)packages/devops/scripts/deployment-aws/startCloudwatchAgent.sh(1 hunks)packages/devops/scripts/deployment-common/startBackEnds.sh(3 hunks)packages/devops/scripts/lambda/resources/startupTupaia.sh(2 hunks)packages/devops/scripts/utility/getEC2TagValue.sh(1 hunks)scripts/bash/backendStartDev.sh(2 hunks)scripts/bash/buildInternalDependencies.sh(2 hunks)scripts/bash/getInternalDependencies.sh(5 hunks)scripts/bash/mergeEnvForDB.sh(2 hunks)scripts/bash/pm2startInline.sh(1 hunks)scripts/bash/validate.sh(1 hunks)scripts/docker/downloadEnvironmentVariables.sh(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/devops/scripts/ci/validateBranchName.sh (1)
packages/devops/scripts/ci/utils.sh (2)
log_error(14-16)get_branch_name(40-48)
🪛 Shellcheck (0.10.0)
packages/devops/scripts/lambda/resources/startupTupaia.sh
[warning] 50-50: sudo doesn't affect redirects. Use ..| sudo tee file
(SC2024)
[warning] 51-51: sudo doesn't affect redirects. Use .. | sudo tee -a file
(SC2024)
scripts/bash/buildInternalDependencies.sh
[warning] 37-37: build_ts_args appears unused. Verify use (or export if used externally).
(SC2034)
[error] 50-50: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 52-52: Argument mixes string and array. Use * or separate argument.
(SC2145)
[warning] 52-52: eval negates the benefit of arrays. Drop eval to preserve whitespace/symbols (or eval as string).
(SC2294)
scripts/bash/getInternalDependencies.sh
[warning] 43-43: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 65-65: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[error] 65-65: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 65-65: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 77-77: Quote the right-hand side of = in [[ ]] to prevent glob matching.
(SC2053)
🔇 Additional comments (111)
packages/devops/scripts/deployment-common/startBackEnds.sh (5)
12-12: Approved: standardized string comparison
The change from==to=in the[[ ... ]]test aligns with Bash best practices and remains functionally equivalent.
13-13: Approved: consistent string comparison
Using=instead of==maintains consistency and portability without altering behavior.
18-18: Approved: single-quoted echo for static text
Switching to single quotes prevents unnecessary interpolation and follows the project's quoting conventions.
31-31: Approved: unified string comparisons in compound test
Consistent use of=in the compound[[ ... ]]expression improves readability and style coherence.
47-47: Approved: echo styling for completion message
Using single quotes for the final status message aligns with other script echoes.packages/devops/scripts/lambda/resources/startupTupaia.sh (2)
28-28: Prefer single '=' for string comparisons
Using=in[[ … ]]is POSIX-compliant and matches our style guidelines, replacing the previous==.
59-59: Use single '=' in pattern matching
Switching from==to=maintains consistency in conditional expressions per our Bash style guide.packages/devops/scripts/deployment-aws/startCloudwatchAgent.sh (1)
3-3: Approve stylistic echo change. Single quotes correctly replace double quotes for a static message, aligning with the PR’s quoting conventions.packages/devops/scripts/deployment-aws/buildDeployablePackages.sh (1)
8-8: Approve echo quoting consistency. Using single quotes for a literal echo message is a clean, shell-safe choice when no variable expansion is needed.packages/devops/scripts/deployment-aws/configureNginx.sh (1)
1-2: Approve shebang and error-exit flags. Switching to/usr/bin/env bashwith an explicitset -eenhances portability and reliability.packages/devops/scripts/utility/getEC2TagValue.sh (1)
1-2: Approve shebang and error-exit flags. Adopting/usr/bin/env bashandset -ealigns this utility with the project’s improved Bash style.packages/devops/scripts/deployment-aws/checkoutLatest.sh (2)
1-2: Portable shebang and strict mode
Switched to#!/usr/bin/env bashand explicitset -e, improving portability and error safety.
24-24: Consistent quoting in echo
Using single quotes for static messages avoids unnecessary interpolation. LGTM.scripts/docker/downloadEnvironmentVariables.sh (3)
37-43: Style update to package selection condition
Theif [[ -z $3 ]]; thenblock correctly uses[[ ]]for safety and single-quoted echo when no interpolation is needed. Approved.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 41-41: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
(SC2124)
58-63: Modernized E2E deployment handling
The conditional using[[ $DEPLOYMENT_NAME = *-e2e || $DEPLOYMENT_NAME = e2e ]]is clear and consistent with project style.
65-71: Dev-specific environment patching
Thedevbranch check and associatedsedinvocation correctly employ[[ ]]and single quotes. Looks good.scripts/bash/pm2startInline.sh (4)
1-2: Consistent shebang and strict mode
Portable#!/usr/bin/env bashwithset -ematches project conventions.
7-10: Improved usage messaging with ANSI styling
Usage check now employs[[ -z $1 ]]and leverages${BOLD},${RESET}, and${UNDERLINE}for clarity. This aligns with centralized ANSI definitions.
14-14: Styled tip message
Consistent use ofecho -ewith ANSI variables for help text. Good enhancement.
22-22: Trap message styling
Using${RED}for emphasis and including[start-stack]prefix standardizes logging on exit. Approved.packages/devops/scripts/ci/validateTypesAndDbSchemaInSync.sh (1)
1-2: Portable shebang with debug flags
Switching to#!/usr/bin/env bashandset -exensures portability and detailed failure output.scripts/bash/validate.sh (2)
1-2: Standard shebang and error handling
Adopting#!/usr/bin/env bashwithset -ealigns with project-wide best practices.
5-5: Quoted sourcing for reliability
Using. "$DIR/../../packages/devops/scripts/ci/utils.sh"correctly handles paths with spaces. Approved.packages/devops/scripts/ci/validateTests.sh (3)
1-2: LGTM: Portable shebang and strict mode. Using/usr/bin/env bashwithset -eenhances portability and ensures early exit on errors.
4-5: LGTM: Path resolution simplification. AssigningROOTrelative to the script’s directory is clear and correctly quoted.
7-7: LGTM: Quoted node script invocation. Quoting the path guards against spaces or special characters.packages/data-api/scripts/refreshAnalyticsTable.sh (2)
1-2: LGTM: Portable shebang and strict mode. Consistent use of/usr/bin/env bashandset -e.
4-4: LGTM: Modern conditional syntax. Using[[ ... ]]with=for string comparison aligns with Bash best practices.packages/data-api/scripts/dropAnalyticsMaterializedView.sh (3)
1-2: LGTM: Portable shebang and strict mode. Standardizing on/usr/bin/env bashandset -eis solid.
5-5: LGTM: Consistent sourcing syntax. Switching to.for sourcing improves portability and readability.
12-13: LGTM: Quoted psql parameters and SQL commands. Proper quoting mitigates splitting and injection risks.packages/data-api/scripts/fastRefreshAnalyticsTable.sh (4)
1-2: LGTM: Portable shebang and strict mode./usr/bin/env bashwithset -eis applied correctly.
4-4: LGTM: Informative echo with single quotes. Single quotes are appropriate since no interpolation is needed.
7-7: LGTM: Consistent sourcing syntax. Using.keeps the style uniform across scripts.
14-14: LGTM: Quoted psql invocation. Proper parameter quoting is in place.packages/data-api/scripts/installMvRefreshModule.sh (4)
1-1: Unsure: Should the shebang be updated to#!/usr/bin/env bashand includeset -efor consistency with other scripts?
10-10: LGTM: Command substitution and SQL quoting. Variables and SQL string are correctly wrapped in quotes.
12-13: LGTM: Conditional test and echo formatting. Using[[ ... ]]and single quotes for static text is consistent.
28-28: LGTM: Quoted psql parameters for SQL script execution. Quoting ensures safe handling of values.packages/data-lake-api/scripts/setupTestDataLake.sh (2)
1-2: Portable shebang and strict mode applied
Good use of#!/usr/bin/env bashand explicitset -eto fail fast on errors.
5-5: Consistent sourcing style
Using.formergeEnvForDB.shis aligned with the PR’s style guide.packages/data-api/scripts/patchMvRefreshModule.sh (3)
1-1: Portable shebang
Updating to#!/usr/bin/env bashis spot-on for portability.
4-5: Consistent sourcing
Sourcing bothmergeEnvForDB.shandansiControlSequences.shwith.matches the style guidelines.
29-29: Quoted psql invocation
Good quoting onpsqlflags and host/db/user parameters.packages/data-api/scripts/fullRefreshAnalyticsTable.sh (4)
1-2: Shebang and fail-fast mode
Using#!/usr/bin/env bashplusset -eis correct and matches project conventions.
4-4: Consistent echo quoting
Switch to single-quoted literal is a safe stylistic change.
7-7: Sourcing environment merge
Shorthand.sourcing is consistent across scripts.
14-16: Well-quoted psql commands
All connection flags and arguments are correctly wrapped in quotes.packages/data-api/scripts/buildAnalyticsMaterializedView.sh (3)
1-2: Standardized shebang and error exit
Aligns with the PR’s portability and robustness goals.
5-5: Consistent environment sourcing
Good use of.formergeEnvForDB.sh.
12-16: Conditional and psql usage
The[[ … ]]test and quotedpsqlinvocations look solid and maintain readability.packages/data-lake-api/scripts/checkTestDataLakeExists.sh (2)
1-2: Shebang and strict mode
Good to see portability andset -efor fail-fast behavior.
5-6: Sourcing ANSI sequences
Bringing in color constants is aligned with the PR objectives.packages/devops/scripts/ci/utils.sh (1)
32-37: Arithmetic comparison and echo usage looks solid
The change to use(( length > max ))and quotingecho "$max"is consistent and reliable.packages/devops/scripts/ci/validateBranchName.sh (3)
14-14: Max branch name length calculation is clear
Using an arithmetic expansion forMAX_BRANCH_NAME_LENGTHis concise and correct.
51-52: Invalid character check is well-quoted
Loop and test now properly quote${INVALID_CHARS[@]}and the pattern, preventing word splitting.
60-62: Function invocations correctly quoted
Calling validation functions with"$branch_name"ensures branch names with spaces (if allowed) are handled safely.scripts/bash/mergeEnvForDB.sh (3)
1-2: Shebang and error exit setup
Switching to#!/usr/bin/env bashwithset -eis more portable and ensures immediate exit on errors.
24-29: Environment files array is correctly defined
Using an array for.envfile paths and quoting entries prevents globbing and word splitting.
46-47: Appending overrides with newlines
Usingmerged_content+="\n$key=\"$value\""cleanly appends each override. Quoting$valueis correct.packages/devops/scripts/ci/triggerRedeploy.sh (7)
1-2: Portable shebang and strict mode enabled
Switching to#!/usr/bin/env bashwithset -eimproves portability and reliability.
8-21: Consistent conditional expression quoting
Using[[ … ]]with quoted variables in the stopped and running instance checks prevents word splitting.
33-36: Robust error detection in Lambda response
grep -q errorMessage "$RESPONSE_FILE"safely checks for errors in the response. Quoting the file is good.
50-58: Maintain quoting in AWS CLI wait and error checks
--instance-ids "$NEW_INSTANCE_ID"and pattern checks in[[ … ]]are properly quoted to avoid splitting.
70-74: Consistent file quoting for Lambda swap response
Quoting"$SWAP_OUT_RESPONSE_FILE"and handling errors withgrepandcatis correct.
79-85: Clear loop for build startup polling
Using(( WAIT_ATTEMPTS++ ))and(( WAIT_ATTEMPTS >= 75 ))in arithmetic contexts is idiomatic Bash.
90-90: Final success message
Single-quoted echo without variable interpolation correctly prints completion status.packages/data-api/scripts/uninstallMvRefreshModule.sh (3)
1-2: Portable shebang with strict mode
Switching to#!/usr/bin/env bashand addingset -ealigns with other scripts for portability and safety.
11-13: SQL existence check is safely quoted
Variables in thepsqlcommand and the[[ … != … ]]test are correctly quoted to prevent splitting.
23-23: PSQL command usage is clear
Quoting$DB_PORT,$DB_URL,$DB_NAME, and$DB_PG_USERfor thepsqlinvocation ensures safe handling of parameters.packages/database/scripts/dumpDatabase.sh (9)
1-2: Unify shebang and error handling
The new portable shebang and explicitset -ecorrectly improve portability and safety.
17-23: Robust spinner invocation
Quoting"$2"in theevaland usingkill -0 "$pid"ensures proper handling of spaces and avoids command injection.
30-31: Reset spinner output cleanly
Usingechoto reset the prompt after the spinner is concise and effective.
34-35: Source environment and ANSI scripts
Sourcing bothmergeEnvForDB.shandansiControlSequences.shin the correct order enables colored output and DB vars—this aligns with the new style.
37-41: Initialize variables with defaults
Variables for dump filename, credentials, server, and target directory are clearly initialized and quoted.
43-49: Modernize argument parsing
Usingwhile [[ $1 != '' ]]and updated[[ ... ]]syntax standardizes option parsing consistently.
60-63: Validate required positional argument
The check for an emptyidentity_filebefore proceeding is correct and user-friendly.
71-74: Validate .env credentials
EnsuringDB_PG_USERandDB_PG_PASSWORDare set before dumping prevents silent failures.
91-92: Inform on completion with color
Echoing the dump path and using${GREEN}and${RESET}nicely highlight success.packages/database/scripts/checkTestDatabaseExists.sh (3)
1-2: Portable shebang and strict mode
Switching to/usr/bin/env bashand addingset -eimproves portability and error handling.
5-6: Source ANSI and Env scripts
LoadingansiControlSequences.shbeforemergeEnvForDB.shensures colored output and required DB vars are available.
15-20: Enhanced error messaging with colors
Replacing raw ANSI codes with${RED},${BOLD}, and${RESET}, along with clear instructions and links, greatly improves user feedback.packages/devops/scripts/ci/validateNewMigrations.sh (12)
1-2: Shebang and strict mode alignment
Updated to/usr/bin/env bashwithset -eto match the unified style.
5-6: Root path and utils sourcing
DefiningROOTand sourcingutils.share correctly quoted and placed.
10-13: Consistent command detection
Using[[ $(uname) = Darwin ]]and echoing'gdate'/'date'aligns with portability improvements.
20-23: Echoing date conversion result
Quoting"$date"inechoensures correct output of computed dates.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 21-21: Declare and assign separately to avoid masking return values.
(SC2155)
38-41: Use arithmetic context for comparisons
The(( ... ))check formigration_timestampis clear and efficient.
51-55: Skip empty migration names robustly
The[[ $migration_name = '' ]]guard cleanly exits the loop on blank lines.
57-60: Aggregate and handle errors
Collectingerrorsand exiting when non-empty maintains script integrity.
64-65: Default origin branch assignment
Explicitly settingorigin_branch_name='master'simplifies CI expectations.
67-70: Skip validation on master branch
Guarding against validating migrations onmasteris the correct behavior.
75-78: Remove SSH submodule for CI
Switching to HTTPS and removing the SSH-based submodule ensures CI won't hang on host authenticity prompts.
80-82: Fetch and validate migrations
Fetching the origin branch and invokingvalidate_migrationsties the flow together correctly.
84-85: Exit success explicitly
Usingexit 0makes the successful validation path explicit.scripts/bash/buildInternalDependencies.sh (5)
1-2: Portable bash invocation
Switched to/usr/bin/env bashandset -efor consistency across scripts.
5-6: Source ANSI styling variables
LoadingansiControlSequences.shenables uniform colored output.
10-11: Informative usage string
Embedding${BOLD}and${RESET}makes the usage message clearer and consistent.
36-38: Initialize build flags
Ternary-style assignments forbuild_argsandbuild_ts_argsare concise.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 37-37: build_ts_args appears unused. Verify use (or export if used externally).
(SC2034)
43-46: Aggregate build commands and prefixes
Populatingbuild_commandsandbuild_prefixesarrays is well-quoted and clear.scripts/bash/backendStartDev.sh (9)
1-7: Standardize shebang and error handling
Puttingset -eon its own line after/usr/bin/env bashaligns with style across scripts.
10-11: Source ANSI styling for messaging
UsingansiControlSequences.shsimplifies bold and reset sequences in echo statements.
12-16: Clear usage declaration
TheUSAGEstring embeds styling cleanly and documents flags properly.
21-28: Consistent flag parsing logic
Using[[ ]]andcasehandles--typescript,--include-internal, and deprecates--skip-internalwith a styled message.
43-45: Switch to TypeScript command when requested
Updatingstart_serverto TS mode withts-node/registeris correct and well-quoted.
47-48: Stylized startup announcement
echo -e "${BOLD}Starting server...${RESET}"gives clear feedback to the user.
50-57: Conditional internal watch flags
Appending towatch_flagsandstart_serverwheninclude_internalistruematches the refactoring in other scripts.
59-60: Inform when skipping internal dependencies
The styled message clearly guides users to the new default behavior.
62-63: Execute with quoting
Usingeval "$start_server"ensures the full command string is parsed correctly.scripts/bash/getInternalDependencies.sh (3)
1-2: Adopt portable shebang and explicit error handling
The shebang is now more portable (#!/usr/bin/env bash) andset -eensures the script exits on any error, matching the PR’s goals.
15-38: Clear listing of default dependencies
Printing each dependency on its own line with backslashes is readable and consistent with other scripts. You could optionally refactor into an array plusprintf '%s\n' "${deps[@]}"for maintainability, but this aligns well with the PR’s stylistic changes.
89-90: Confirm robust output quoting
Usingecho "${internal_dependencies[@]}"safely preserves each element, aligning with the PR’s emphasis on proper quoting.
| \. "$HOME_DIR/.nvm/nvm.sh" # Load nvm so node is available on $PATH | ||
| sudo -u ubuntu echo "10 13 * * * PATH=$PATH $HOME_DIR/tupaia/packages/web-config-server/run_preaggregation.sh | while IFS= read -r line; do printf '\%s \%s\\n' \"\$(date)\" \"\$line\"; done > $LOGS_DIR/preaggregation.txt" > tmp.cron | ||
| sudo -u ubuntu crontab -l >> tmp.cron || echo "" >> tmp.cron | ||
| sudo -u ubuntu crontab -l >> tmp.cron || echo >> tmp.cron |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unsure: Redirection may not run under sudo, leading to crontab -l failing to append
The >> tmp.cron redirection is handled by the current shell (root), not the ubuntu user, so sudo -u ubuntu crontab -l >> tmp.cron can fail on permission grounds and always hit the fallback—dropping existing entries. Consider running the entire command (including its redirect) under sudo.
-sudo -u ubuntu crontab -l >> tmp.cron || echo >> tmp.cron
+sudo -u ubuntu bash -lc "crontab -l >> tmp.cron" || sudo -u ubuntu bash -lc "echo >> tmp.cron"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sudo -u ubuntu crontab -l >> tmp.cron || echo >> tmp.cron | |
| sudo -u ubuntu bash -lc "crontab -l >> tmp.cron" || sudo -u ubuntu bash -lc "echo >> tmp.cron" |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 51-51: sudo doesn't affect redirects. Use .. | sudo tee -a file
(SC2024)
🤖 Prompt for AI Agents
In packages/devops/scripts/lambda/resources/startupTupaia.sh at line 51, the
redirection operator >> tmp.cron is executed by the root shell, not by the
ubuntu user, causing permission issues when appending the output of sudo -u
ubuntu crontab -l. To fix this, run the entire command including the redirection
under sudo by wrapping it in a shell executed by sudo, such as using sudo -u
ubuntu sh -c 'crontab -l >> tmp.cron', ensuring the redirection happens with the
correct user permissions.
| : "${DATA_LAKE_DB_PORT:=5432}" | ||
|
|
||
| TUPAIA_USER_EXISTS=`PGPASSWORD=$DB_PG_PASSWORD psql -p $DATA_LAKE_DB_PORT -X -A -h $DATA_LAKE_DB_URL -U $DB_PG_USER -t -c "SELECT rolname FROM pg_catalog.pg_roles WHERE rolname = '$DATA_LAKE_DB_USER'"` | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use modern command substitution and quote expansions
Replace backticks with $(…) and quote variables to handle any special characters:
-TUPAIA_USER_EXISTS=`PGPASSWORD=$DB_PG_PASSWORD psql -p $DATA_LAKE_DB_PORT -X -A -h $DATA_LAKE_DB_URL -U $DB_PG_USER -t -c "…"`
+TUPAIA_USER_EXISTS=$(PGPASSWORD="$DB_PG_PASSWORD" psql -p "$DATA_LAKE_DB_PORT" -X -A -h "$DATA_LAKE_DB_URL" -U "$DB_PG_USER" -t -c "…")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TUPAIA_USER_EXISTS=$(PGPASSWORD="$DB_PG_PASSWORD" psql -p "$DATA_LAKE_DB_PORT" -X -A -h "$DATA_LAKE_DB_URL" -U "$DB_PG_USER" -t -c "…") |
🤖 Prompt for AI Agents
In packages/data-lake-api/scripts/setupTestDataLake.sh at line 11, replace any
command substitutions using backticks with the modern $(...) syntax and ensure
all variable expansions are quoted to properly handle special characters and
prevent word splitting or globbing issues.
| echo "No patches exist for version: $VERSION, skipping" | ||
| else | ||
| ts-node ./scripts/patchMvRefresh.ts $COMMAND:$VERSION $PATCH_NAME --migrations-dir "./scripts/patches" --table "patches" -v --config-file "../../babel.config.json" | ||
| ts-node ./scripts/patchMvRefresh.ts $COMMAND:$VERSION $PATCH_NAME --migrations-dir './scripts/patches' --table 'patches' -v --config-file '../../babel.config.json' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote variables in ts-node call
Avoid issues with spaces:
-ts-node ./scripts/patchMvRefresh.ts $COMMAND:$VERSION $PATCH_NAME …
+ts-node ./scripts/patchMvRefresh.ts "$COMMAND:$VERSION" "$PATCH_NAME" …📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ts-node ./scripts/patchMvRefresh.ts $COMMAND:$VERSION $PATCH_NAME --migrations-dir './scripts/patches' --table 'patches' -v --config-file '../../babel.config.json' | |
| ts-node ./scripts/patchMvRefresh.ts "$COMMAND:$VERSION" "$PATCH_NAME" --migrations-dir './scripts/patches' --table 'patches' -v --config-file '../../babel.config.json' |
🤖 Prompt for AI Agents
In packages/data-api/scripts/patchMvRefreshModule.sh at line 42, the variables
used in the ts-node command are not quoted, which can cause issues if they
contain spaces. To fix this, wrap the variables $COMMAND:$VERSION and
$PATCH_NAME in double quotes to ensure they are treated as single arguments and
prevent word splitting or globbing problems.
| fi | ||
|
|
||
| if [[ ! -d "./scripts/patches/$VERSION" && ! "$COMMAND" == "create" ]]; then | ||
| if [[ ! -d ./scripts/patches/$VERSION && $COMMAND != create ]]; then |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote path and variables in directory check
Prevent globbing and word-splitting:
-if [[ ! -d ./scripts/patches/$VERSION && $COMMAND != create ]]; then
+if [[ ! -d "./scripts/patches/$VERSION" && $COMMAND != create ]]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ ! -d ./scripts/patches/$VERSION && $COMMAND != create ]]; then | |
| if [[ ! -d "./scripts/patches/$VERSION" && $COMMAND != create ]]; then |
🤖 Prompt for AI Agents
In packages/data-api/scripts/patchMvRefreshModule.sh at line 39, the directory
check condition uses unquoted variables and paths, which can cause globbing and
word-splitting issues. To fix this, quote the path and the variable references
inside the test brackets, changing the line to use "./scripts/patches/$VERSION"
and "$COMMAND" in quotes to ensure proper evaluation and prevent unexpected
behavior.
| echo # reset prompt | ||
| } | ||
|
|
||
| DIR=$(pwd "$0") |
There was a problem hiding this comment.
Critical: Incorrect script directory resolution
DIR=$(pwd "$0") is invalid—pwd doesn’t accept a script path. Use something like:
DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"🤖 Prompt for AI Agents
In packages/database/scripts/dumpDatabase.sh at line 33, the current assignment
DIR=$(pwd "$0") is incorrect because pwd does not take arguments. Replace this
line with DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" to correctly
resolve the script's directory path.
| if (( ${#internal_dependencies[@]} = 0 )); then | ||
| exit 0 # no internal dependencies of this package, early return | ||
| fi |
There was a problem hiding this comment.
Fix arithmetic conditional to comparison operator
Using = inside (( ... )) performs assignment, not comparison. Update to == to correctly test for zero entries:
- if (( ${#internal_dependencies[@]} = 0 )); then
+ if (( ${#internal_dependencies[@]} == 0 )); then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (( ${#internal_dependencies[@]} = 0 )); then | |
| exit 0 # no internal dependencies of this package, early return | |
| fi | |
| if (( ${#internal_dependencies[@]} == 0 )); then | |
| exit 0 # no internal dependencies of this package, early return | |
| fi |
🤖 Prompt for AI Agents
In scripts/bash/getInternalDependencies.sh around lines 44 to 46, the
conditional uses a single equals sign inside the arithmetic evaluation, which
performs assignment instead of comparison. Change the single equals sign `=` to
a double equals sign `==` to correctly compare the length of the
internal_dependencies array to zero.
| nested_dependencies=($(${DIR}/getInternalDependencies.sh "${package_path}/../${dependency}" ${dependencies_already_visited[@]} ${internal_dependencies[@]} )) | ||
| if [ ${#nested_dependencies[@]} -eq 0 ]; then | ||
| if (( ${#nested_dependencies[@]} = 0 )); then | ||
| continue | ||
| fi | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Safely populate nested_dependencies from command output
Populating arrays with nested_dependencies=($(…)) can split on whitespace and glob. Prefer mapfile -t (or read -a) with quoted expansions:
- nested_dependencies=($(${DIR}/getInternalDependencies.sh "${package_path}/../${dependency}" ${dependencies_already_visited[@]} ${internal_dependencies[@]} ))
+ mapfile -t nested_dependencies < <("${DIR}/getInternalDependencies.sh" \
+ "${package_path}/../${dependency}" \
+ "${dependencies_already_visited[@]}" \
+ "${internal_dependencies[@]}")This avoids unintended splitting and globbing.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| nested_dependencies=($(${DIR}/getInternalDependencies.sh "${package_path}/../${dependency}" ${dependencies_already_visited[@]} ${internal_dependencies[@]} )) | |
| if [ ${#nested_dependencies[@]} -eq 0 ]; then | |
| if (( ${#nested_dependencies[@]} = 0 )); then | |
| continue | |
| fi | |
| mapfile -t nested_dependencies < <("${DIR}/getInternalDependencies.sh" \ | |
| "${package_path}/../${dependency}" \ | |
| "${dependencies_already_visited[@]}" \ | |
| "${internal_dependencies[@]}") | |
| if (( ${#nested_dependencies[@]} == 0 )); then | |
| continue | |
| fi |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 65-65: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[error] 65-65: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 65-65: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🤖 Prompt for AI Agents
In scripts/bash/getInternalDependencies.sh around lines 65 to 69, the
nested_dependencies array is populated using command substitution with unquoted
expansions, which can cause word splitting and globbing issues. Replace the
current assignment with a safer approach using mapfile -t or read -a combined
with quoted command substitution to correctly read each line of output into the
array without splitting or globbing.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
scripts/bash/getInternalDependencies.sh (2)
44-46: Fix comparison operator in arithmetic test
The single=assigns rather than compares. Change to==to properly check for an empty array.
65-69: Handle nested dependencies safely
The unquotednested_dependencies=($(…))approach can lead to splitting/globbing, and the=in(( … ))should be==. Consider usingmapfile -tas above and updating the comparison operator.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 65-65: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[error] 65-65: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 65-65: Double quote array expansions to avoid re-splitting elements.
(SC2068)
🧹 Nitpick comments (6)
scripts/bash/mergeEnvForDB.sh (1)
24-29: Unsure: these relative paths assume the script is invoked from its own directory; prefixing with"$CURRENT_DIR"or$CALLING_SCRIPT_DIR(or using absolute$REPO_ROOT) would guarantee correct file resolution.scripts/bash/buildInternalDependencies.sh (2)
50-52: Unsure: mixing array expansions inside a string passed toevalmay lead to splitting issues; consider building a command array and invoking it directly instead ofeval.🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 50-50: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 52-52: Argument mixes string and array. Use * or separate argument.
(SC2145)
[warning] 52-52: eval negates the benefit of arrays. Drop eval to preserve whitespace/symbols (or eval as string).
(SC2294)
55-57: Unsure: same pattern in the non-watch branch—avoidingevaland using an array invocation would prevent quoting pitfalls.packages/devops/scripts/ci/validateNewMigrations.sh (1)
46-46: Avoid masking return codes
Declaring and assigning in one line (local foo=$(...)) masks the exit status of the command; consider splitting declaration and assignment.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 46-46: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/bash/getInternalDependencies.sh (2)
5-10: Prefer-nfor non-empty check
You can simplifyif [[ $1 != '' ]]toif [[ -n $1 ]]for clarity.
77-77: Quote comparison operands
Wrap the right-hand side in quotes to prevent globbing:[[ ${internal_dependencies[i]} = "${internal_dependencies[j]}" ]].🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 77-77: Quote the right-hand side of = in [[ ]] to prevent glob matching.
(SC2053)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
packages/devops/scripts/ci/validateBranchName.sh(3 hunks)packages/devops/scripts/ci/validateNewMigrations.sh(2 hunks)packages/devops/scripts/ci/validateTests.sh(1 hunks)packages/devops/scripts/deployment-aws/buildDeployablePackages.sh(1 hunks)packages/devops/scripts/deployment-aws/checkoutLatest.sh(2 hunks)packages/devops/scripts/deployment-aws/configureNginx.sh(1 hunks)packages/devops/scripts/deployment-aws/startCloudwatchAgent.sh(1 hunks)packages/devops/scripts/deployment-common/startBackEnds.sh(4 hunks)packages/devops/scripts/deployment-vm-app/install.sh(1 hunks)packages/web-config-server/run_preaggregation.sh(1 hunks)scripts/bash/backendStartDev.sh(2 hunks)scripts/bash/buildInternalDependencies.sh(2 hunks)scripts/bash/buildPackagesByGlob.sh(1 hunks)scripts/bash/downloadEnvironmentVariables.sh(1 hunks)scripts/bash/getInternalDependencies.sh(5 hunks)scripts/bash/getPackagesWithEnvFiles.sh(1 hunks)scripts/bash/mergeEnvForDB.sh(2 hunks)scripts/bash/validate.sh(1 hunks)scripts/docker/downloadEnvironmentVariables.sh(3 hunks)
✅ Files skipped from review due to trivial changes (4)
- packages/devops/scripts/deployment-aws/buildDeployablePackages.sh
- scripts/bash/buildPackagesByGlob.sh
- packages/devops/scripts/deployment-vm-app/install.sh
- packages/web-config-server/run_preaggregation.sh
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/devops/scripts/deployment-aws/startCloudwatchAgent.sh
- packages/devops/scripts/ci/validateTests.sh
- packages/devops/scripts/deployment-common/startBackEnds.sh
- scripts/bash/validate.sh
- scripts/docker/downloadEnvironmentVariables.sh
- packages/devops/scripts/deployment-aws/configureNginx.sh
- packages/devops/scripts/ci/validateBranchName.sh
- packages/devops/scripts/deployment-aws/checkoutLatest.sh
- scripts/bash/backendStartDev.sh
🧰 Additional context used
🪛 Shellcheck (0.10.0)
packages/devops/scripts/ci/validateNewMigrations.sh
[warning] 46-46: Declare and assign separately to avoid masking return values.
(SC2155)
scripts/bash/buildInternalDependencies.sh
[warning] 37-37: build_ts_args appears unused. Verify use (or export if used externally).
(SC2034)
[error] 50-50: Argument mixes string and array. Use * or separate argument.
(SC2145)
[error] 52-52: Argument mixes string and array. Use * or separate argument.
(SC2145)
[warning] 52-52: eval negates the benefit of arrays. Drop eval to preserve whitespace/symbols (or eval as string).
(SC2294)
scripts/bash/getInternalDependencies.sh
[warning] 43-43: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[warning] 65-65: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
[error] 65-65: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[error] 65-65: Double quote array expansions to avoid re-splitting elements.
(SC2068)
[warning] 77-77: Quote the right-hand side of = in [[ ]] to prevent glob matching.
(SC2053)
scripts/bash/mergeEnvForDB.sh
[warning] 21-21: CURRENT_DIR appears unused. Verify use (or export if used externally).
(SC2034)
🔇 Additional comments (31)
scripts/bash/downloadEnvironmentVariables.sh (1)
5-5: Robust script directory resolution
Replacing the olddirname "$0"withBASH_SOURCE+cd+pwdensures you get an absolute path (even through symlinks).scripts/bash/getPackagesWithEnvFiles.sh (1)
4-4: Consistent directory calculation
Switching toBASH_SOURCEwithcdandpwdstandardizes how the script finds its own location across contexts.scripts/bash/mergeEnvForDB.sh (6)
1-2: Portable shebang & explicit error handling
Using#!/usr/bin/env bashwith a separateset -ematches the project convention and improves portability.
8-8: Use[[ ... ]]for loop condition
Adopting[[ $dir != '/' ]]is more idiomatic than[ ... ]and handles edge cases better.
21-21: Unsure ifCURRENT_DIRis ever referenced later; consider removing it or exporting it if needed to avoid unused-variable warnings.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 21-21: CURRENT_DIR appears unused. Verify use (or export if used externally).
(SC2034)
32-38: Only cat existing files
Iterating over the array and checking-fbeforecatprevents errors when some.envfiles aren’t present.
45-48: Preserve override syntax safely
Appending eachkey="value"on its own line with proper quoting prevents word splitting and ensures overrides are valid.
52-52: Unsure: usingeval "$merged_content"can be risky if untrusted content gets in; consider writing to a temporary file and sourcing it or sanitizing inputs first.scripts/bash/buildInternalDependencies.sh (5)
1-2: Standardize shebang & error exit
Switching to#!/usr/bin/env bashwithset -ealigns this script with others in the repo.
4-5: Centralize ANSI styling
SourcingansiControlSequences.shavoids inline escape codes and unifies the look across scripts.
10-10: Enhanced usage message
Embedding${BOLD}/${RESET}in the$USAGEstring makes help output more readable and consistent.
14-14: Idiomatic argument loop
Usingwhile [[ $1 != '' ]]is clearer and safer than mixing[ ... ]with shifts.
36-37: Unsure ifbuild_ts_argsis ever used downstream; remove it or wire it into commands to avoid dead-code noise.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 37-37: build_ts_args appears unused. Verify use (or export if used externally).
(SC2034)
packages/devops/scripts/ci/validateNewMigrations.sh (13)
1-2: Portable shebang with strict mode
Changing to/usr/bin/env bash+set -eimproves portability and ensures failures aren’t ignored.
4-6: Consistent path setup
ResolvingDIRviaBASH_SOURCEand definingROOTensures the script operates from known locations.
7-7: Reuse CI utilities
Sourcingutils.shcentralizes common functions likeget_branch_nameand logging helpers.
10-14: Cross-platformdatedetection
DetectingDarwinforgdateinstallation support makes timestamp logic portable between macOS and Linux.
22-23: Quote inecho
Switching toecho "$date"avoids word splitting and is good practice—even for simple strings.
28-29: Unsure: the subtraction and division order might yield milliseconds instead of seconds; consider parentheses for clarity:inc_ts=$(( $(date +%s) - (included_date_offset/1000) ))
33-34: Unsure: hardcoded substring offsets assume exact migration-name length; adding a regex check before slicing could prevent silent failures on unexpected filenames.
37-37: Use arithmetic context
Using(( ... ))for numeric comparison is concise and avoids the pitfalls of[ ... ].
42-47: Localize function parameters
Defininglocalfor function args and using here-strings to read new migration names keeps variable scope tight.🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 46-46: Declare and assign separately to avoid masking return values.
(SC2155)
49-55: Here-string loop for new migrations
Reading viawhile read -rand a here-string is neat and avoids spawning subshells for each file.
56-59: Exit on validation failures
Aggregating errors and exiting non-zero fulfills CI expectations to fail on bad migrations.
66-69: Skip validation on master
Preventing redundant checks when branches match streamlines CI run time.
75-76: Switch remote & remove SSH submodule
Updating the Git remote to HTTPS and removing the SSH-based submodule ensures CI fetches work without SSH keys.scripts/bash/getInternalDependencies.sh (5)
1-2: Portable shebang & strict mode
Good update: switched to a portable shebang and enabledset -efor better error handling.
4-4: Use BASH_SOURCE for script path resolution
Nice use of${BASH_SOURCE[0]}andcdto reliably determineDIR.
15-38: List internal dependencies for readability
Breaking the echo arguments across lines improves maintainability without changing behavior.
63-63: Good array iteration quoting
Usingfor dependency in "${internal_dependencies[@]}"safely handles items with spaces.
89-89: Consistent output quoting
Usingecho "${internal_dependencies[@]}"correctly preserves each element.
| fi | ||
|
|
||
| # we are getting internal dependencies for a specific package.json | ||
| internal_dependencies=($(sed -n '/"dependencies": {/,/}/p' ${PWD}/${package_path}/package.json | grep -o '@tupaia/[^"]*": "[0-9\.]*"' | cut -d / -f 2 | cut -d \" -f 1)) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor array population to avoid word splitting
Unsure if this might be wrong, but consider replacing internal_dependencies=($(…)) with a mapfile -t or read -a approach and quoting the command substitution to prevent globbing and splitting:
-internal_dependencies=($(sed -n '/"dependencies": {/,/}/p' "${PWD}/${package_path}/package.json" \
- | grep -o '@tupaia/[^"]*": "[0-9\.]*"' \
- | cut -d / -f 2 | cut -d '"' -f 1))
+mapfile -t internal_dependencies < <(sed -n '/"dependencies": {/,/}/p' "${PWD}/${package_path}/package.json" \
+ | grep -o '@tupaia/[^"]*": "[0-9\.]*"' \
+ | cut -d / -f 2 | cut -d '"' -f 1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| internal_dependencies=($(sed -n '/"dependencies": {/,/}/p' ${PWD}/${package_path}/package.json | grep -o '@tupaia/[^"]*": "[0-9\.]*"' | cut -d / -f 2 | cut -d \" -f 1)) | |
| # Replace the array assignment with a safe `mapfile` approach | |
| mapfile -t internal_dependencies < <(sed -n '/"dependencies": {/,/}/p' "${PWD}/${package_path}/package.json" \ | |
| | grep -o '@tupaia/[^"]*": "[0-9\.]*"' \ | |
| | cut -d / -f 2 | cut -d '"' -f 1) |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 43-43: Prefer mapfile or read -a to split command output (or quote to avoid splitting).
(SC2207)
🤖 Prompt for AI Agents
In scripts/bash/getInternalDependencies.sh at line 43, the array
internal_dependencies is populated using unquoted command substitution, which
can cause word splitting and globbing issues. To fix this, replace the current
assignment with a safer approach using mapfile -t or read -a to read the output
lines into the array, and ensure the command substitution is properly quoted to
prevent unwanted splitting or globbing.
# Conflicts: # scripts/bash/buildInternalDependencies.sh # scripts/bash/downloadEnvironmentVariables.sh # scripts/bash/getInternalDependencies.sh
# Conflicts: # packages/devops/scripts/deployment-aws/buildDeployablePackages.sh # scripts/bash/backendStartDev.sh
# Conflicts: # packages/devops/scripts/ci/utils.sh # packages/devops/scripts/ci/validateBranchName.sh # packages/devops/scripts/ci/validateNewMigrations.sh # packages/devops/scripts/deployment-common/startBackEnds.sh # scripts/bash/downloadEnvironmentVariables.sh # scripts/bash/getPackagesWithEnvFiles.sh
# Conflicts: # packages/devops/scripts/ci/triggerRedeploy.sh # packages/devops/scripts/lambda/resources/startupTupaia.sh
1 participant
Took some time over the weekend to read up on Bash. This ended up being a logical place for me to apply some of those learnings. Completely understandable if no one wants to review this, especially considering it touches some deployment scripts.
Tip
This is one to review commit-by-commit.
I’ve only made changes to shell scripts specifically targeting Bash. Plain
shscripts are untouched. Changes are largely informed by ShellCheck, shellharden and BashPitfalls.Changes
Note
Other than prettifying a few outputs to the terminal, this is a pretty pure refactor with no material/logical/algorithmic changes.
#!/usr/bin/env bashover#!/bin/bash.-lis set[[ ... ]].${...}when expanding variables unless meaningfully more readable (or necessary, like in"${foo}bar").$(...)over`...`for command substitutions.[[ ... ]]over[ ... ]for tests.(( ... ))over[[ ... ]]for arithmetic comparisons.=over==in tests.+=syntax for concatenation of strings and arrays..oversourcecommand.sourceeminently more readable, but.is more portable. However, in Bash, they are equivalent, so there is a real argument for us to prefersourcein Bash scripts. ❗ Open to opinions!ansiControlSequences.shfor convenience variables like${RED}and${RESET}for prettifying stdout. This honours theNO_COLORenvironment variable if the user has it set.download-env-vars,backendStartDev.shanddumpDatabase.sh,patchMvRefereshModule.sh.(Sorry about all the force pushes. I squashed a lot of commits to try and keep the commit history clean 😅)