fix: failing backup and restore appsmithctl #35162
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe recent updates enhance the database URL retrieval process in the application by introducing the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Utils as Utils Module
participant Config as Configuration File
participant Env as Environment Variables
App->>Utils: Call getDburl()
Utils->>Env: Check APPSMITH_DB_URL
alt If defined
Utils-->>App: Return APPSMITH_DB_URL
else If not defined
Utils->>Config: Read from ENV_PATH
Config-->>Utils: Provide DB URL
Utils-->>App: Return DB URL
end
Assessment against linked issues
Poem
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 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: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- deploy/docker/fs/opt/appsmith/utils/bin/backup.js (3 hunks)
- deploy/docker/fs/opt/appsmith/utils/bin/constants.js (2 hunks)
- deploy/docker/fs/opt/appsmith/utils/bin/utils.js (3 hunks)
Additional context used
Biome
deploy/docker/fs/opt/appsmith/utils/bin/utils.js
[error] 49-49: Shouldn't redeclare 'dbUrl'. Consider to delete it or rename it.
'dbUrl' is defined here:
(lint/suspicious/noRedeclare)
Additional comments not posted (6)
deploy/docker/fs/opt/appsmith/utils/bin/constants.js (2)
12-13: Good addition ofENV_PATHconstant.The
ENV_PATHconstant provides a clear reference to the Docker environment configuration file, which enhances maintainability.
29-30: Proper inclusion ofENV_PATHin module exports.Including
ENV_PATHin the module exports makes it accessible to other modules, promoting reusability.deploy/docker/fs/opt/appsmith/utils/bin/utils.js (2)
5-5: Correct inclusion of thefsmodule.The
fsmodule is necessary for file operations within the newgetDburlfunction.
197-198: Proper inclusion ofgetDburlin module exports.Including
getDburlin the module exports makes it accessible to other modules, promoting reusability.deploy/docker/fs/opt/appsmith/utils/bin/backup.js (2)
128-128: Good use ofutils.getDburl()inexportDatabase.Using
utils.getDburl()centralizes the logic for obtaining the database URL, enhancing maintainability.
144-144: Good use ofutils.getDburl()increateManifestFile.Using
utils.getDburl()centralizes the logic for obtaining the database URL, enhancing maintainability.
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- deploy/docker/fs/opt/appsmith/utils/bin/restore.js (3 hunks)
Additional comments not posted (2)
deploy/docker/fs/opt/appsmith/utils/bin/restore.js (2)
Line range hint
60-71:
Great job on improving the flexibility of therestoreDatabasefunction!The changes to accept
dbUrlas a parameter and use it in themongorestorecommand enhance the function's adaptability. Ensure that thedbUrlparameter is correctly passed and sanitized to prevent any security vulnerabilities.
212-213: Nice update to therunfunction!The changes to pass
utils.getDburl()as an argument torestoreDatabasealign well with the updated function signature. Ensure thatutils.getDburl()returns a valid and sanitized database URL.
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between c32725f and c4e22957ae130ef010b60e6f035a03ead5be837d.
Files selected for processing (1)
- deploy/docker/fs/opt/appsmith/utils/bin/utils.js (3 hunks)
Additional comments not posted (2)
deploy/docker/fs/opt/appsmith/utils/bin/utils.js (2)
198-198: Confirm the addition ofgetDburlto module exports.The
getDburlfunction has been correctly added to the module exports.
48-52: Use a fallback mechanism for environment variables.Ensure that the fallback mechanism for environment variables is properly implemented.
- let dbEnvUrl = process.env.APPSMITH_DB_URL || process.env.APPSMITH_MONGO_DB_URI; - if (dbEnvUrl && dbEnvUrl !== "undefined") { - dbUrl = dbEnvUrl; - } + const dbEnvUrl = process.env.APPSMITH_DB_URL || process.env.APPSMITH_MONGO_DB_URI; + if (dbEnvUrl && dbEnvUrl !== "undefined") { + dbUrl = dbEnvUrl; + }Likely invalid or redundant comment.
| try { | ||
| let env_array = fs.readFileSync(Constants.ENV_PATH, 'utf8').toString().split("\n"); | ||
| for (let i in env_array) { | ||
| if (env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL")) { | ||
| dbUrl = env_array[i].toString().split("=")[1]; | ||
| break; // Break early when the desired line is found | ||
| } | ||
| } | ||
| } catch (err) { | ||
| console.error("Error reading the environment file:", err); | ||
| } |
There was a problem hiding this comment.
Avoid redeclaring variables and optimize the loop.
The dbUrl variable is redeclared, which can be avoided by declaring it once and updating its value conditionally. Additionally, the loop can be optimized by breaking early when the desired line is found.
- let dbUrl;
- try {
- let env_array = fs.readFileSync(Constants.ENV_PATH, 'utf8').toString().split("\n");
- for (let i in env_array) {
- if (env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL")) {
- dbUrl = env_array[i].toString().split("=")[1];
- break; // Break early when the desired line is found
- }
- }
- } catch (err) {
- console.error("Error reading the environment file:", err);
- }
+ let dbUrl = '';
+ try {
+ let env_array = fs.readFileSync(Constants.ENV_PATH, 'utf8').toString().split("\n");
+ for (let i in env_array) {
+ if (env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL")) {
+ dbUrl = env_array[i].toString().split("=")[1];
+ break; // Break early when the desired line is found
+ }
+ }
+ } catch (err) {
+ console.error("Error reading the environment file:", err);
+ }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.
| try { | |
| let env_array = fs.readFileSync(Constants.ENV_PATH, 'utf8').toString().split("\n"); | |
| for (let i in env_array) { | |
| if (env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL")) { | |
| dbUrl = env_array[i].toString().split("=")[1]; | |
| break; // Break early when the desired line is found | |
| } | |
| } | |
| } catch (err) { | |
| console.error("Error reading the environment file:", err); | |
| } | |
| let dbUrl = ''; | |
| try { | |
| let env_array = fs.readFileSync(Constants.ENV_PATH, 'utf8').toString().split("\n"); | |
| for (let i in env_array) { | |
| if (env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL")) { | |
| dbUrl = env_array[i].toString().split("=")[1]; | |
| break; // Break early when the desired line is found | |
| } | |
| } | |
| } catch (err) { | |
| console.error("Error reading the environment file:", err); | |
| } |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10253260440. |
pratapaprasanna
force-pushed
the
fix/failing/backup/restore
branch
from
c4e2295 to
1f89992
Compare
pratapaprasanna
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Commits
Files that changed from the base of the PR and between c4e22957ae130ef010b60e6f035a03ead5be837d and 1f89992.
Files selected for processing (1)
- deploy/docker/fs/opt/appsmith/utils/bin/utils.js (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- deploy/docker/fs/opt/appsmith/utils/bin/utils.js
|
Deploy-Preview-URL: https://ce-35162.dp.appsmith.com |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- deploy/docker/fs/opt/appsmith/utils/bin/export_db.js (2 hunks)
- deploy/docker/fs/opt/appsmith/utils/bin/import_db.js (2 hunks)
- deploy/docker/fs/opt/appsmith/utils/bin/restore.js (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- deploy/docker/fs/opt/appsmith/utils/bin/restore.js
Additional comments not posted (4)
deploy/docker/fs/opt/appsmith/utils/bin/export_db.js (2)
4-4: Good practice: Importing utility functions.Importing the
utilsmodule helps in maintaining cleaner and more modular code.
8-8: Dynamic DB URL retrieval.Using
utils.getDburl()to fetch the database URL dynamically enhances flexibility and maintainability.deploy/docker/fs/opt/appsmith/utils/bin/import_db.js (2)
6-6: Good practice: Importing utility functions.Importing the
utilsmodule helps in maintaining cleaner and more modular code.
11-11: Dynamic DB URL retrieval.Using
utils.getDburl()to fetch the database URL dynamically enhances flexibility and maintainability.
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/10264398607. |
|
Deploy-Preview-URL: https://ce-35162.dp.appsmith.com |
github-actions
bot
added
Backup & Restore
| let dbEnvUrl = process.env.APPSMITH_DB_URL || process.env.APPSMITH_MONGO_DB_URI; | ||
| if (dbEnvUrl && dbEnvUrl !== "undefined") { | ||
| dbUrl = dbEnvUrl; | ||
| } |
There was a problem hiding this comment.
Utility functions should generally be pure and env variables should not be a part of it. If you were to drive the utility function the env values as a parameter is okay, however i think you can move this function into a separate file and not be a part of the utility file.
There was a problem hiding this comment.
Merging this for the time being because of the urgency. I will address this in the next release
thanks @vsvamsi1
| @@ -31,6 +32,26 @@ function start(apps) { | |||
| console.log("Started " + appsStr); | |||
| } | |||
|
|
|||
| function getDburl() { | |||
| let dbUrl = ''; | |||
There was a problem hiding this comment.
I have a nitpick comment.
It appears to me that dbUrl variable job is to be just eventually returned, i think we can remove this and directly the return the result.
There was a problem hiding this comment.
An example
function getDburl() {
try {
const env_array = fs.readFileSync(Constants.ENV_PATH, 'utf8').toString().split("\n");
for (let i in env_array) {
if (env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL")) {
// return early when the desired line is found
return env_array[i].toString().split("=")[1];
}
}
} catch (err) {
console.error("Error reading the environment file:", err);
}
}
let dbEnvUrl = process.env.APPSMITH_DB_URL || process.env.APPSMITH_MONGO_DB_URI;
if (dbEnvUrl && dbEnvUrl !== "undefined") {
return dbEnvUrl;
}
return "";
}
Fixes: #34996
Summary by CodeRabbit