chore: Add polling for embedded pg start to create appsmith DB #36854
Conversation
WalkthroughThe pull request introduces modifications to 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: 0
🧹 Outside diff range and nitpick comments (1)
deploy/docker/fs/opt/appsmith/entrypoint.sh (1)
470-478: Class, let's examine this excellent improvement to our PostgreSQL initialization!The addition of a maximum attempts mechanism for waiting on PostgreSQL to start is a commendable enhancement. It prevents potential infinite loops and provides more informative logging. Well done!
However, I'd like to suggest a small improvement to make our code even better:
Consider extracting the
max_attemptsvalue into a configurable variable at the beginning of the script. This would allow easier adjustment of the timeout period without modifying the function itself. Here's an example:+ # Near the top of the script + POSTGRES_MAX_INIT_ATTEMPTS=${POSTGRES_MAX_INIT_ATTEMPTS:-100} # In the create_appsmith_pg_db function - local max_attempts=100 + local max_attempts=$POSTGRES_MAX_INIT_ATTEMPTSThis change would make our script more flexible and easier to maintain. Remember, class, good code is always evolving!
## Description PR to add the polling for `pg_isready` signal for embedded postgres DB. This will fix the infinite loop where postgres refuse to come up. We have seen this failure earlier with `openshift` setup. Current PR will make sure when the embedded postgres fails to startup we are not copying the postgres.conf file as well so supervisor won't have to start the postgres process at all. Ref: https://theappsmith.slack.com/archives/C0341RERY4R/p1728565913269689?thread_ts=1728544650.663739&cid=C0341RERY4R /test Sanity ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11322083108> > Commit: df82790 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11322083108&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Mon, 14 Oct 2024 08:17:55 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced database initialization process with a maximum attempts mechanism for waiting on PostgreSQL to start. - Improved configuration handling for `APPSMITH_DB_URL` based on database type. - **Bug Fixes** - Ensured robust error handling if PostgreSQL fails to start within the specified attempts. - **Documentation** - Added comments for improved clarity in the deployment script. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit 348a58f)
Description
PR to add the polling for
pg_isreadysignal for embedded postgres DB. This will fix the infinite loop where postgres refuse to come up. We have seen this failure earlier withopenshiftsetup. Current PR will make sure when the embedded postgres fails to startup we are not copying the postgres.conf file as well so supervisor won't have to start the postgres process at all.Ref: https://theappsmith.slack.com/archives/C0341RERY4R/p1728565913269689?thread_ts=1728544650.663739&cid=C0341RERY4R
/test Sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11322083108
Commit: df82790
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 14 Oct 2024 08:17:55 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
APPSMITH_DB_URLbased on database type.Bug Fixes
Documentation