chore: Add postgres dependency for server to startup #36585
Conversation
|
Warning Rate limit exceeded@abhvsn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes introduce two new functions in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- deploy/docker/fs/opt/appsmith/run-java.sh (2 hunks)
🧰 Additional context used
🪛 Shellcheck
deploy/docker/fs/opt/appsmith/run-java.sh
[warning] 81-96: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (2)
deploy/docker/fs/opt/appsmith/run-java.sh (2)
19-64: Well done implementing the PostgreSQL availability check functionYou've done a great job with the
waitForPostgresAvailabilityfunction. It effectively checks the database status and includes proper error handling with retries. This is essential for ensuring the server starts reliably, even when the PostgreSQL database is recovering.
120-123: Great job integrating PostgreSQL parameter extraction and availability checkingBy adding this conditional block, you're ensuring that the script properly handles PostgreSQL configurations. Extracting the database parameters and verifying the database's availability before proceeding demonstrates good attention to detail and improves the robustness of the startup process.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
deploy/docker/fs/opt/appsmith/pg-utils.sh (1)
61-88: Implement Validation for Extracted ParametersWhile your function extracts the database parameters, it's prudent to validate these parameters before proceeding. This helps in catching any issues with the connection string early and prevents potential runtime errors.
Consider adding validation checks like:
if [ -z "$PG_DB_HOST" ] || [ -z "$PG_DB_NAME" ]; then tlog "Error: Missing essential database parameters. Please verify the connection string." exit 1 fiThis ensures that the script only continues if all necessary parameters are present.
🧰 Tools
🪛 Shellcheck
[warning] 65-80: Quote this to prevent word splitting.
(SC2046)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- deploy/docker/fs/opt/appsmith/pg-utils.sh (1 hunks)
🧰 Additional context used
🪛 Shellcheck
deploy/docker/fs/opt/appsmith/pg-utils.sh
[warning] 65-80: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (3)
deploy/docker/fs/opt/appsmith/pg-utils.sh (3)
3-48: Good Job on Implementing PostgreSQL Availability CheckYou've done well to create the
waitForPostgresAvailabilityfunction. This ensures that the PostgreSQL server is ready before the application proceeds, which is essential for maintaining reliability and preventing connection issues. The use of a retry mechanism with proper logging demonstrates sound scripting practices.
13-13: Verify Permissions forsu postgresCommandWhen using
su postgres -c "pg_isready...", it's important to consider the script's execution environment. Thesucommand requires the script to run with sufficient privileges; otherwise, it may fail due to permission issues.I recommend checking whether the script will always have the necessary permissions in your deployment scenarios. If not, you might explore alternatives like adjusting user permissions or using
sudowith appropriate configurations.
90-92:⚠️ Potential issueComment Out Example Usage to Prevent Unintended Execution
Including examples is a great way to document how to use your functions. However, it's important to ensure these examples don't execute unintentionally.
Please modify the example usage as follows:
-# Example usage of the functions -# waitForPostgresAvailability -# extract_postgres_db_params "postgresql://user:password@localhost:5432/dbname" +# Example usage of the functions +# waitForPostgresAvailability +# extract_postgres_db_params "postgresql://user:password@localhost:5432/dbname"By commenting out the lines fully, you prevent the sample code from running if the script is executed, while still providing valuable examples to other developers.
Likely invalid or redundant comment.
| IFS=' ' read -r USER PASSWORD HOST PORT DB <<<$(node -e " | ||
| const connectionString = process.argv[1]; | ||
| const pgUri = connectionString.startsWith(\"postgresql://\") | ||
| ? connectionString | ||
| : 'http://' + connectionString; //Prepend a fake scheme for URL parsing | ||
| const url = require('url'); | ||
| const parsedUrl = new url.URL(pgUri); | ||
|
|
||
| // Extract the pathname and remove the leading '/' | ||
| const db = parsedUrl.pathname.substring(1); | ||
|
|
||
| // Default the port to 5432 if it's empty | ||
| const port = parsedUrl.port || '5432'; | ||
|
|
||
| console.log(\`\${parsedUrl.username || '-'} \${parsedUrl.password || '-'} \${parsedUrl.hostname} \${port} \${db}\`); | ||
| " "$conn_string") |
There was a problem hiding this comment.
Quote Command Substitution to Prevent Word Splitting
In the extract_postgres_db_params function, the output of the node command should be enclosed in quotes to prevent word splitting. Without quotes, parameters containing spaces could lead to incorrect parsing of database credentials.
Here's how you can fix it:
- IFS=' ' read -r USER PASSWORD HOST PORT DB <<<$(node -e "
+ IFS=' ' read -r USER PASSWORD HOST PORT DB <<<"$(node -e "
const connectionString = process.argv[1];
const pgUri = connectionString.startsWith(\"postgresql://\")
? connectionString
: 'http://' + connectionString; // Prepend a fake scheme for URL parsing
const url = require('url');
const parsedUrl = new url.URL(pgUri);
// Extract the pathname and remove the leading '/'
const db = parsedUrl.pathname.substring(1);
// Default the port to 5432 if it's empty
const port = parsedUrl.port || '5432';
console.log(\`\${parsedUrl.username || '-'} \${parsedUrl.password || '-'} \${parsedUrl.hostname} \${port} \${db}\`);
- " "$conn_string")
+ " "$conn_string")"By quoting the command substitution, you ensure that all the variables are read correctly, even if they contain spaces.
📝 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.
| IFS=' ' read -r USER PASSWORD HOST PORT DB <<<$(node -e " | |
| const connectionString = process.argv[1]; | |
| const pgUri = connectionString.startsWith(\"postgresql://\") | |
| ? connectionString | |
| : 'http://' + connectionString; //Prepend a fake scheme for URL parsing | |
| const url = require('url'); | |
| const parsedUrl = new url.URL(pgUri); | |
| // Extract the pathname and remove the leading '/' | |
| const db = parsedUrl.pathname.substring(1); | |
| // Default the port to 5432 if it's empty | |
| const port = parsedUrl.port || '5432'; | |
| console.log(\`\${parsedUrl.username || '-'} \${parsedUrl.password || '-'} \${parsedUrl.hostname} \${port} \${db}\`); | |
| " "$conn_string") | |
| IFS=' ' read -r USER PASSWORD HOST PORT DB <<<"$(node -e " | |
| const connectionString = process.argv[1]; | |
| const pgUri = connectionString.startsWith(\"postgresql://\") | |
| ? connectionString | |
| : 'http://' + connectionString; //Prepend a fake scheme for URL parsing | |
| const url = require('url'); | |
| const parsedUrl = new url.URL(pgUri); | |
| // Extract the pathname and remove the leading '/' | |
| const db = parsedUrl.pathname.substring(1); | |
| // Default the port to 5432 if it's empty | |
| const port = parsedUrl.port || '5432'; | |
| console.log(\`\${parsedUrl.username || '-'} \${parsedUrl.password || '-'} \${parsedUrl.hostname} \${port} \${db}\`); | |
| " "$conn_string")" |
🧰 Tools
🪛 Shellcheck
[warning] 65-80: Quote this to prevent word splitting.
(SC2046)
There was a problem hiding this comment.
@abhvsn Can you please modify the code to resolve this comment?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@mohanarpit I am not able to understand the advantage for this. I have written the tests to see the effect of this change but couldn't find anything, tests are written for following APPSMITH_DB_URL input for which the response was exactly same with or without quotes:
- postgresql://user:password@localhost:5432/dbname
- postgresql://user:p a s s w o r d@localhost:5432/dbname
- postgresql://user:password@localhost:5432/db name
There was a problem hiding this comment.
https://github.com/coderabbitai can you please check the above comment, if I'm missing something?
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11070194243. |
|
Deploy-Preview-URL: https://ce-36585.dp.appsmith.com |
…add-postgres-dependency-for-server
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
deploy/docker/tests/test_pg-utils.sh (1)
48-54: Let's enhance our debugging function for even more clarity.The
echo_paramsfunction is a great addition for debugging! It's always important to have tools that help us understand what's happening when things don't go as expected.To make this even more helpful, let's add a little more context. How about we modify it like this:
echo_params() { echo "Extracted PostgreSQL parameters:" echo "--------------------------------" echo "PG_DB_USER: $PG_DB_USER" echo "PG_DB_PASSWORD: $PG_DB_PASSWORD" echo "PG_DB_HOST: $PG_DB_HOST" echo "PG_DB_PORT: $PG_DB_PORT" echo "PG_DB_NAME: $PG_DB_NAME" echo "--------------------------------" }Can anyone tell me why adding this extra context might be beneficial when we're debugging our tests?
Description
As in the past we have seen the corruption of postgres DB which is being used for temporal we want to make sure we have a retry mechanism in place if:
APPSMITH_DB_URLis pointing to postgres urlAs per local testing when the docker container is abruptly shutdown via
docker rm -f {container_name}ordocker kill {container_name}or even via docker desktop we end up in state where postgres goes into recovery state.logs:
Currently we have implemented polling mechanism, but we will keep looking for better alternative here if we can opt for.
Note:
Reference doc: https://www.notion.so/appsmith/Postgres-critical-scenarios-668f49c96aef40308e24c2a8d6b1137c
/test Sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11100944184
Commit: 9dbbe4b
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 30 Sep 2024 07:26:41 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Summary by CodeRabbit