chore: Remove shelljs from more appsmithctl commands
#37383
Conversation
WalkthroughThe changes in this pull request involve significant updates to various scripts related to backup, export, import, and utility functions within the application. Key modifications include transitioning from synchronous to asynchronous operations, enhancing error handling, and improving logging practices. The removal of 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
|
sharat87
changed the title
shelljs from more appsmithctl commandsshelljs from more appsmithctl commands
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11833719737. |
|
Deploy-Preview-URL: https://ce-37383.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: 1
🧹 Outside diff range and nitpick comments (17)
deploy/docker/fs/opt/appsmith/utils/bin/import_db.js (5)
Line range hint
1-6: Replace shelljs with native Node.js modulesThe PR objective is to remove shelljs dependency, but this file still uses it. Consider using native Node.js alternatives:
child_process.execSyncfor shell commandsfsfor file operationsExample refactor:
-const shell = require('shelljs'); +const { execSync } = require('child_process');
Line range hint
8-13: Add error handling and sanitize database URLThe database import function needs improvements:
- Add try-catch for error handling
- Sanitize dbUrl to prevent command injection
- Check mongorestore exit code
Example refactor:
function import_database() { console.log('import_database ....') - dbUrl = utils.getDburl(); - const cmd = `mongorestore --uri='${dbUrl}' --drop --archive='${Constants.RESTORE_PATH}/${Constants.DUMP_FILE_NAME}' --gzip` - shell.exec(cmd) + try { + const dbUrl = utils.getDburl(); + const sanitizedUrl = dbUrl.replace(/[;&|`$]/g, ''); + const result = execSync( + `mongorestore --uri="${sanitizedUrl}" --drop --archive="${Constants.RESTORE_PATH}/${Constants.DUMP_FILE_NAME}" --gzip`, + { encoding: 'utf8' } + ); + if (result.status !== 0) { + throw new Error(`mongorestore failed with exit code ${result.status}`); + } + } catch (error) { + console.error('Database import failed:', error); + throw error; + } console.log('import_database done') }
Line range hint
15-21: Add error handling for supervisor commandsThe application lifecycle functions should verify command execution and handle errors.
Example refactor:
function stop_application() { - shell.exec('/usr/bin/supervisorctl stop backend rts') + try { + const result = execSync('/usr/bin/supervisorctl stop backend rts', { encoding: 'utf8' }); + if (!result.includes('stopped')) { + throw new Error('Failed to stop applications'); + } + } catch (error) { + console.error('Failed to stop applications:', error); + throw error; + } }
33-34: Use async/await for supervisor checkReplace callback with modern async/await pattern.
- shell.exec(check_supervisord_status_cmd, function (code) { - if (code > 0) { - console.log('application is not running, starting supervisord') - shell.exec('/usr/bin/supervisord') - } - }) + const supervisorStatus = await checkSupervisorStatus(); + if (!supervisorStatus) { + console.log('application is not running, starting supervisord'); + await startSupervisor(); + }
Line range hint
47-58: Simplify user confirmation logicThe nested conditional logic for user confirmation could be simplified.
- console.log() - console.log('**************************** WARNING ****************************') - console.log(`Your target database is not empty, it has data in ${collectionsLen} collections.`) - const input = readlineSync.question('Importing this DB will erase this data. Are you sure you want to proceed?[Yes/No] ') - const answer = input && input.toLocaleUpperCase() - if (answer === 'Y' || answer === 'YES') { - import_database() - return - } else if (answer === 'N' || answer === 'NO') { - return - } + const warning = [ + '', + '**************************** WARNING ****************************', + `Your target database is not empty, it has data in ${collectionsLen} collections.` + ].join('\n'); + console.log(warning); + + const answer = readlineSync.question('Importing this DB will erase this data. Are you sure you want to proceed?[Yes/No] ') + .toLocaleUpperCase(); + + if (['Y', 'YES'].includes(answer)) { + return import_database(); + } + if (['N', 'NO'].includes(answer)) { + return; + }🧰 Tools
🪛 Biome
[error] 51-51: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
deploy/docker/fs/opt/appsmith/utils/bin/mailer.js (4)
Line range hint
17-29: Simplify email configuration validation logicThe mailEnabled check is redundant as it appears in both the first condition and an else-if branch. Consider consolidating the validation logic.
- if (!mailEnabled || !mailFrom || !mailHost || !mailPort || !mailUser || !mailPass) { + if (!mailFrom || !mailHost || !mailPort || !mailUser || !mailPass) { throw new Error('Failed to send error mail. Email provider is not configured, please refer to https://docs.appsmith.com/setup/instance-configuration/email to configure it.'); } else if (!mailTo) { throw new Error('Failed to send error mail. Admin email(s) not configured, please refer to https://docs.appsmith.com/setup/instance-configuration/disable-user-signup#administrator-emails to configure it.'); } - else if (!mailEnabled) { + if (!mailEnabled) { throw new Error('Mail not sent! APPSMITH_MAIL_ENABLED env val is disabled, please refer to https://docs.appsmith.com/setup/instance-configuration/email to enable it.'); } - else {
Line range hint
14-69: Improve error handling and propagationThe catch block swallows errors after logging them. Consider propagating errors to allow proper handling by calling code.
} catch (err) { await logger.backup_error(err.stack); + throw err; }
Line range hint
41-54: Use template literals and secure protocol for URLsTwo suggestions for improvement:
- Use template literals for better readability
- Use https:// instead of http:// for admin settings URL
- let text = 'Appsmith backup did not complete successfully.\n\n ' + - 'Backup timestamp: ' + backupTimestamp + '\n\n' + - 'Last Successful Backup timestamp: ' + lastBackupTimestamp + '\n' + - 'Last Successful Backup location: ' + lastBackupPath + '\n\n'; + let text = `Appsmith backup did not complete successfully. + +Backup timestamp: ${backupTimestamp} + +Last Successful Backup timestamp: ${lastBackupTimestamp} +Last Successful Backup location: ${lastBackupPath} +`; if (instanceName) { - text = text + 'Appsmith instance name: ' + instanceName + '\n'; + text += `Appsmith instance name: ${instanceName}\n`; } if (domainName) { - text = text + 'Link to Appsmith admin settings: ' + 'http://' + domainName + '/settings/general' + '\n'; + text += `Link to Appsmith admin settings: https://${domainName}/settings/general\n`; } - text = text + '\n' + err.stack; + text += `\n${err.stack}`;
Line range hint
56-65: Add logging for email sending statusConsider adding logging to track successful email delivery.
await transporter.sendMail({ from: mailFrom, to: mailTo, subject: '[Appsmith] ERROR: Backup Failed', text: text - }); + }).then(() => { + logger.info('Backup error notification email sent successfully'); + });deploy/docker/fs/opt/appsmith/utils/bin/utils.js (1)
20-27: Enhance error message specificityThe error message could be more helpful by including the specific error details from the caught exception.
- console.error('Supervisor is not running, exiting.'); + console.error('Supervisor is not running:', e.message);deploy/docker/fs/opt/appsmith/utils/bin/backup.js (3)
48-58: Fix typo in encryption error messageThe error message contains a typo: "enctyption" should be "encryption".
- throw new Error('Backup process aborted because a valid enctyption password could not be obtained from the user'); + throw new Error('Backup process aborted because a valid encryption password could not be obtained from the user');
48-58: Consider adding debug logging for encryption processAdd debug logging to help troubleshoot encryption issues in production.
if (encryptArchive){ + await logger.backup_debug('Starting backup encryption...'); const encryptedArchivePath = await encryptBackupArchive(archivePath,encryptionPassword); + await logger.backup_debug('Encryption completed successfully'); await logger.backup_info('Finished creating an encrypted a backup archive at ' + encryptedArchivePath);
Line range hint
95-98: Consider using environment variable for encryption passwordPassing encryption password via command line to openssl is visible in process list. Consider using environment variable instead.
- await utils.execCommand(['openssl', 'enc', '-aes-256-cbc', '-pbkdf2', '-iter', 100000, '-in', archivePath, '-out', encryptedArchivePath, '-k', encryptionPassword ]) + process.env.BACKUP_ENCRYPTION_PWD = encryptionPassword; + await utils.execCommand(['openssl', 'enc', '-aes-256-cbc', '-pbkdf2', '-iter', 100000, '-in', archivePath, '-out', encryptedArchivePath, '-pass', 'env:BACKUP_ENCRYPTION_PWD']) + delete process.env.BACKUP_ENCRYPTION_PWD;deploy/docker/fs/opt/appsmith/utils/bin/restore.js (1)
Line range hint
178-206: Add error logging for critical failuresWhile the error handling is good, consider adding detailed logging for critical failures to aid in troubleshooting.
Add error logging like this:
} catch (err) { - console.log(err); + console.error('Critical error during restore process:', { + error: err.message, + stack: err.stack, + phase: 'restore' + });deploy/docker/fs/opt/appsmith/utils/bin/export_db.js (3)
27-27: Adjust log message to reflect actual restart timingThe services are restarted in the
finallyblock, not immediately after this log message. Consider updating the log message for clarity.Apply this diff to update the log message:
- console.log('start backend & rts application after export database'); + console.log('Backend and RTS applications will be restarted after export completion');
38-38: Use 'console.error' for error loggingReplace
console.logwithconsole.errorwhen logging errors to ensure they are properly captured in error logs.Apply this diff to improve error logging:
} catch (err) { - console.log(err); + console.error(err); errorCode = 1; } finally {
48-49: Question: Is exporting 'exportDatabase' necessary?If
exportDatabaseis not used externally, consider removing it from the exports to keep the module interface clean.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js(5 hunks)deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js(0 hunks)deploy/docker/fs/opt/appsmith/utils/bin/export_db.js(1 hunks)deploy/docker/fs/opt/appsmith/utils/bin/import_db.js(3 hunks)deploy/docker/fs/opt/appsmith/utils/bin/mailer.js(1 hunks)deploy/docker/fs/opt/appsmith/utils/bin/restore.js(3 hunks)deploy/docker/fs/opt/appsmith/utils/bin/utils.js(3 hunks)
💤 Files with no reviewable changes (1)
- deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js
🧰 Additional context used
📓 Learnings (1)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js (1)
Learnt from: sharat87
PR: appsmithorg/appsmith#37356
File: deploy/docker/fs/opt/appsmith/utils/bin/backup.js:242-242
Timestamp: 2024-11-13T09:25:45.218Z
Learning: In `backup.js`, prefer to keep error messages simple without adding calculations for constant values if the hardcoded value suffices.
🔇 Additional comments (12)
deploy/docker/fs/opt/appsmith/utils/bin/import_db.js (1)
Line range hint 1-76: Verify logging consistency across related files
Let's ensure the logging changes are consistent with other utility files.
✅ Verification successful
Logging patterns are consistent across utility files
The logging patterns in import_db.js align with the rest of the utility files in the codebase. All files consistently use console.log for operational status messages, warnings, and error reporting. The logging style and verbosity level match across related files like export_db.js, backup.js, and restore.js.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check logging patterns in related files
rg -A 1 "shell\.echo|console\.log" deploy/docker/fs/opt/appsmith/utils/bin/
Length of output: 20877
deploy/docker/fs/opt/appsmith/utils/bin/mailer.js (1)
71-71: LGTM!
The trailing comma addition follows modern JavaScript conventions.
deploy/docker/fs/opt/appsmith/utils/bin/utils.js (3)
29-38: LGTM! Clean async implementation
The conversion to async/await is well-implemented with proper logging and error propagation.
Line range hint 195-206: LGTM! Exports are complete and well-organized
All new functions are properly exported while maintaining existing functionality.
Line range hint 195-206: Verify command execution implementations in the codebase
The command execution implementations look robust. Let's verify their usage across the codebase to ensure consistent error handling.
deploy/docker/fs/opt/appsmith/utils/bin/backup.js (2)
19-19: LGTM! Clear improvement in function naming
The change from shelljs exec to a dedicated supervisor check function improves code clarity.
191-192: LGTM! Clean implementation using native Node.js APIs
Good use of fs.promises.mkdtemp with os.tmpdir() for temporary directory creation.
deploy/docker/fs/opt/appsmith/utils/bin/restore.js (2)
206-206: Verify graceful shutdown of services
Ensure that the services have completely stopped before proceeding with restore operations.
Let's verify the service management implementation:
#!/bin/bash
# Check the stop/start implementation for graceful handling
ast-grep --pattern 'async function stop($_) {
$$$
}'Also applies to: 221-221
176-177: LGTM: Supervisor check properly implemented
Good addition of the supervisor check before proceeding with restoration.
Let's verify the supervisor check implementation:
✅ Verification successful
Supervisor check implementation is robust and well-implemented
The ensureSupervisorIsRunning function is properly implemented in utils.js with:
- Appropriate error handling
- Silent check using
supervisorctl - Clear error message if supervisor is not running
- Proper async/await usage
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the supervisor check implementation
ast-grep --pattern 'async function ensureSupervisorIsRunning() {
$$$
}'
Length of output: 571
Script:
#!/bin/bash
# Let's check for the utils.js file content to find the supervisor check implementation
rg -A 5 "ensureSupervisorIsRunning" deploy/docker/fs/opt/appsmith/utils/bin/utils.js
# Also check for any supervisor-related functions
rg -A 5 "supervisor" deploy/docker/fs/opt/appsmith/utils/bin/utils.js
Length of output: 940
deploy/docker/fs/opt/appsmith/utils/bin/export_db.js (3)
5-14: LGTM for asynchronous 'exportDatabase' function
The conversion of exportDatabase to an asynchronous function and the use of native promises enhance the code's readability and maintainability.
41-43: Confirmed: Services restart handled correctly
Good use of the finally block to ensure that services are restarted even if an error occurs during the export.
1-1: Verify Node.js version compatibility for 'fs/promises'
Ensure that the production environment uses a Node.js version that supports the fs/promises module.
Run the following script to check the Node.js version:
✅ Verification successful
Node.js version is compatible with fs/promises
The production environment uses Node.js v22.9.0 which fully supports the fs/promises module (supported since v14.x).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if Node.js version is 14.x or higher.
node -v
Length of output: 17
| console.log(err) | ||
| errorCode = 1 | ||
| } finally { | ||
| shell.echo('start backend & rts application after import database') | ||
| console.log('start backend & rts application after import database') | ||
| start_application() | ||
| process.exit(errorCode) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in finally block
The error handling and application restart logic could be more robust.
} catch (err) {
- console.log(err)
+ console.error('Import failed:', err)
errorCode = 1
} finally {
- console.log('start backend & rts application after import database')
- start_application()
- process.exit(errorCode)
+ try {
+ console.log('Restarting applications...')
+ start_application()
+ console.log('Applications restarted successfully')
+ } catch (error) {
+ console.error('Failed to restart applications:', error)
+ errorCode = 1
+ } finally {
+ process.exit(errorCode)
+ }
}📝 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.
| console.log(err) | |
| errorCode = 1 | |
| } finally { | |
| shell.echo('start backend & rts application after import database') | |
| console.log('start backend & rts application after import database') | |
| start_application() | |
| process.exit(errorCode) | |
| } | |
| console.error('Import failed:', err) | |
| errorCode = 1 | |
| } finally { | |
| try { | |
| console.log('Restarting applications...') | |
| start_application() | |
| console.log('Applications restarted successfully') | |
| } catch (error) { | |
| console.error('Failed to restart applications:', error) | |
| errorCode = 1 | |
| } finally { | |
| process.exit(errorCode) | |
| } | |
| } |
This PR removes more uses of
shelljsfrom theappsmithctlproject, towards completely removing that dependency. Then we should be able to buildappsmithctlwithesbuild, and reduce false-positive CVEs being reported fromappsmithctl'snode_modulesfolder.Automation
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11833714062
Commit: d63aa15
Cypress dashboard.
Tags:
@tag.SanitySpec:
Thu, 14 Nov 2024 09:21:47 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores