chore: Remove shelljs use from backup command
#37356
Conversation
WalkthroughThe changes in this pull request primarily focus on enhancing the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (4)deploy/docker/fs/opt/appsmith/utils/bin/utils.js (4)
Setting Line range hint The 'close' event is more reliable as it ensures all stdio streams are flushed and closed.
Good improvement passing the actual error object instead of just rejecting. Line range hint The implementation would benefit from additional safeguards as mentioned in the previous review:
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
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11811194576. |
|
Deploy-Preview-URL: https://ce-37356.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11811591015. |
|
Deploy-Preview-URL: https://ce-37356.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: 5
🧹 Outside diff range and nitpick comments (6)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js (2)
20-25: LGTM! Consider enhancing the error message.The supervisor check implementation is correct, but the error message could be more helpful.
- console.error('Supervisor is not running, exiting.'); + console.error('Supervisor service is not running. Please ensure the supervisor service is started before running backup.');
Line range hint
1-263: Excellent modernization of the backup utility.The transition from shelljs to native Node.js features is well-executed. The consistent use of async/await, proper error handling, and secure backup encryption make this a robust implementation.
Consider adding retry mechanisms for critical operations like database export and archive creation, as these operations might fail due to temporary system conditions.
deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js (4)
15-18: Consider using a test-specific directory instead of root pathUsing "/" as the test path might cause issues in different environments. Consider using a test-specific directory or mocking
fsPromises.statfs.- const res = expect(await backup.getAvailableBackupSpaceInBytes("/")) + const testDir = os.tmpdir(); + const res = expect(await backup.getAvailableBackupSpaceInBytes(testDir))
24-24: Fix test naming typos and improve claritySeveral test names contain typos or are unclear:
- "Checkx" should be "Checks"
- "hould" is duplicated
- "Generates t" is incomplete
-it('Checkx the constant is 2 GB', () => { +it('Checks if the constant is 2 GB', () => { -it('Should not hould throw Error when the available size is >= MIN_REQUIRED_DISK_SPACE_IN_BYTES', () => { +it('Should not throw Error when the available size is >= MIN_REQUIRED_DISK_SPACE_IN_BYTES', () => { -it('Generates t', async () => { +it('Generates temporary backup path', async () => {Also applies to: 30-30, 33-33
Line range hint
147-157: Remove duplicate test caseThis test case is duplicated. The second occurrence can be removed as it's identical to the previous test.
-test('Cleanup Backups when limit is 4 and there are 2 files', async () => { - const backupArchivesLimit = 4; - fsPromises.rm = jest.fn().mockImplementation(async (a) => console.log(a)); - var backupFiles = ['file1','file2'] - var expectedBackupFiles = ['file1','file2'] - const res = await backup.removeOldBackups(backupFiles,backupArchivesLimit) - console.log(res) - expect(res).toEqual(expectedBackupFiles) -})
Line range hint
10-13: Remove console.log statements from testsDebug console.log statements should be removed from test cases. They add noise to test output and don't provide value in production code.
Also applies to: 116-119, 147-157, 190-193
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js(3 hunks)deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js(1 hunks)deploy/docker/fs/opt/appsmith/utils/bin/constants.js(2 hunks)deploy/docker/fs/opt/appsmith/utils/bin/utils.js(4 hunks)deploy/docker/fs/opt/appsmith/utils/bin/utils.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- deploy/docker/fs/opt/appsmith/utils/bin/constants.js
🔇 Additional comments (5)
deploy/docker/fs/opt/appsmith/utils/bin/utils.test.js (1)
1-3: LGTM! Clean and focused imports.
deploy/docker/fs/opt/appsmith/utils/bin/utils.js (2)
186-186: LGTM! Improved error handling and process management.
Good improvements:
- Using "close" event ensures all streams are closed
- Passing error object provides better debugging context
- Ignoring stdio is appropriate for silent execution
Also applies to: 189-189, 206-206
221-221: LGTM! Export addition follows existing pattern.
deploy/docker/fs/opt/appsmith/utils/bin/backup.js (2)
235-237: LGTM! Efficient implementation using native fs promises.
The change from shell commands to native Node.js fs promises is a good improvement for performance and reliability.
263-263: LGTM! All necessary functions are exported.
The module exports are complete and well-organized.
This is towards fixing
appsmithctlinto a modern component of the project. First order of business is to get rid of theshelljsdependency, since we don't really need it that seriously, and since it does weird stuff withrequire()that we can't useesbuildto buildappsmithctl.Further PRs will remove
shelljsfrom other commands as well, and then we'll removeshelljsfrom our dependencies.Automation
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11814325698
Commit: 98261ee
Cypress dashboard.
Tags:
@tag.SanitySpec:
Wed, 13 Nov 2024 09:58:36 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation