fix: DB URL override fails in some cases#34206
Conversation
WalkthroughThe recent updates ensure that the environment variable Changes
Sequence Diagram(s)The changes are straightforward and do not significantly alter the control flow, so no sequence diagrams are necessary. Possibly related issues
Poem
Warning Review ran into problemsProblems (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9480979769. |
|
Deploy-Preview-URL: https://ce-34206.dp.appsmith.com |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (2)
deploy/docker/fs/opt/appsmith/utils/bin/index.js (1)
Line range hint
33-33: Refactor to move the return statements inside the respective command functions to avoid illegal return statements outside of a function.- return; + // Move the return statement inside the respective functions.Also applies to: 42-42, 47-47, 52-52, 57-57, 61-61
Tools
Biome
[error] 16-16: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
deploy/docker/fs/opt/appsmith/entrypoint.sh (1)
Line range hint
62-62: Declare and assign separately to avoid masking return values, which can lead to hidden bugs.- local generated_appsmith_mongodb_password=$(tr -dc A-Za-z0-9 </dev/urandom | head -c 13; echo "") + local generated_appsmith_mongodb_password + generated_appsmith_mongodb_password=$(tr -dc A-Za-z0-9 </dev/urandom | head -c 13; echo "")Also applies to: 66-66, 70-70, 74-74
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- deploy/docker/fs/opt/appsmith/entrypoint.sh (3 hunks)
- deploy/docker/fs/opt/appsmith/utils/bin/index.js (1 hunks)
Additional context used
Biome
deploy/docker/fs/opt/appsmith/utils/bin/index.js
[error] 33-33: Illegal return statement outside of a function (parse)
[error] 42-42: Illegal return statement outside of a function (parse)
[error] 47-47: Illegal return statement outside of a function (parse)
[error] 52-52: Illegal return statement outside of a function (parse)
[error] 57-57: Illegal return statement outside of a function (parse)
[error] 61-61: Illegal return statement outside of a function (parse)
[error] 16-16: Avoid the delete operator which can impact performance. (lint/performance/noDelete)
Unsafe fix: Use an undefined assignment instead.
Shellcheck
deploy/docker/fs/opt/appsmith/entrypoint.sh
[warning] 62-62: Declare and assign separately to avoid masking return values. (SC2155)
[warning] 66-66: Declare and assign separately to avoid masking return values. (SC2155)
[warning] 70-70: Declare and assign separately to avoid masking return values. (SC2155)
[warning] 74-74: Declare and assign separately to avoid masking return values. (SC2155)
[warning] 86-86: ShellCheck can't follow non-constant source. Use a directive to specify location. (SC1090)
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- deploy/docker/fs/opt/appsmith/utils/bin/index.js (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- deploy/docker/fs/opt/appsmith/utils/bin/index.js
3 participants
The current fallback implementation doesn't work in the below case:
This scenario will be showing up a lot more now that the
docker.env.shthat generates newdocker.envfiles hasAPPSMITH_DB_URLin it.Problem is that since we load env variables from both outside and
docker.envindividually, we end up loading bothAPPSMITH_MONGODB_URIandAPPSMITH_DB_URL. And in this case, theAPPSMITH_DB_URLwill be from the just-generateddocker.env, so we'll end up with a localhost URL, even thoughAPPSMITH_MONGODB_URIwas set to an external endpoint outside the container.This is the problem we were facing with our DPs recently.
This PR fixes this problem by doing the env name "rename" separately for outside env variables, and once for
docker.envenv variables./test sanity
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9481037167
Commit: c6ce2a8
Cypress dashboard url: Click here!
Summary by CodeRabbit
APPSMITH_DB_URLis correctly set, enhancing database connection reliability.