chore: Move all remaining actions into links #37829
Conversation
WalkthroughThe pull request introduces significant changes to the backup functionality across multiple files. Key modifications include the refactoring 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
|
github-actions
bot
added
the
skip-changelog
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12080117884. |
|
Deploy-Preview-URL: https://ce-37829.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: 10
🧹 Outside diff range and nitpick comments (10)
app/client/packages/rts/src/ctl/backup/BackupState.ts (1)
3-3: Consider extracting timestamp generation to a utility function.Direct string manipulation in the class makes the code less maintainable. Consider moving this to a utility function for better reusability and easier testing.
+// In a separate utils.ts file +export const generateBackupTimestamp = () => + new Date().toISOString().replace(/:/g, "-"); -readonly initAt: string = new Date().toISOString().replace(/:/g, "-"); +readonly initAt: string = generateBackupTimestamp();app/client/packages/rts/src/ctl/backup/links/BackupFolderLink.ts (2)
7-10: Consider enhancing JSDoc documentation.While the documentation is clear, consider adding TypeScript-friendly JSDoc tags for better IDE integration.
/** + * @class + * @implements {Link} * Creates the backup folder in pre step, and deletes it in post step. The existence of the backup folder should only * be assumed in the "doBackup" step, and no other. */
11-26: Consider implementing cleanup safety mechanism.The current implementation might leave orphaned temporary directories if the process crashes between pre and post backup steps.
Consider implementing one of these solutions:
- Register a process-level cleanup handler
- Store and clean up old temporary directories on startup
- Use a cleanup service that runs periodically
Would you like me to provide an implementation for any of these approaches?
app/client/packages/rts/src/ctl/backup/links/GitStorageLink.ts (2)
6-8: Enhance class documentationWhile the current documentation describes what the class does, it would be beneficial to include:
22-28: Consider environment-specific configuration and path validationThe hardcoded default path might not work across different environments. Consider:
- Making the default path configurable
- Validating that the path exists before returning
export function getGitRoot(gitRoot?: string | undefined) { + const defaultPath = process.env.DEFAULT_GIT_ROOT || "/appsmith-stacks/git-storage"; if (gitRoot == null || gitRoot === "") { - gitRoot = "/appsmith-stacks/git-storage"; + gitRoot = defaultPath; } + if (!utils.fileExists(gitRoot)) { + throw new Error(`Git root path ${gitRoot} does not exist`); + } return gitRoot; }app/client/packages/rts/src/ctl/backup/links/EnvFileLink.ts (1)
43-48: Consider enhancing the warning message delivery.The warning message could be more visible and persistent for better user awareness.
Consider this enhancement:
async postBackup() { if (!this.state.isEncryptionEnabled) { - console.log(SECRETS_WARNING); + console.warn(SECRETS_WARNING); + // Consider writing this warning to a separate file in the backup + await fsPromises.writeFile( + this.state.backupRootPath + "/BACKUP_WARNINGS.txt", + SECRETS_WARNING, + { flag: 'a' } + ); } }app/client/packages/rts/src/ctl/backup/backup.test.ts (2)
63-63: Consider parameterizing command generation testsThe MongoDB and copy command tests could benefit from parameterization to test multiple scenarios.
Consider using test.each:
test.each([ ['/dest', 'mongodb://username:password@host/appsmith', 'mongodump --uri=mongodb://username:password@host/appsmith --archive=/dest/mongodb-data.gz --gzip'], // Add more test cases ])('Test mongodump CMD generation with %s', async (dest, uri, expected) => { const res = await executeMongoDumpCMD(dest, uri); expect(res).toBe(expected); });Also applies to: 85-85
106-106: Add test cases for edge cases in sensitive data removalWhile the basic cases are covered, consider adding tests for:
- Empty input
- Input with only sensitive data
- Mixed case sensitive keys
Also applies to: 115-115
app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts (2)
26-29: Include the backup path in the error messageIn
checkAvailableBackupSpace, pass thepathparameter to include the actual backup path in the error message.Apply this diff to implement the change:
export function checkAvailableBackupSpace(availSpaceInBytes: number, path: string) { if (availSpaceInBytes < Constants.MIN_REQUIRED_DISK_SPACE_IN_BYTES) { throw new Error( - "Not enough space available at /appsmith-stacks. Please ensure availability of at least 2GB to backup successfully.", + `Not enough space available at ${path}. Please ensure at least 2GB of free space to backup successfully.`, ); } }
Line range hint
9-12: Parameterize the backup path instead of hardcodingHardcoding the path
"/appsmith-stacks"reduces flexibility. Retrieve this path from a configuration or constant.Apply this diff to parameterize the path:
async preBackup() { + const backupPath = Constants.BACKUP_PATH; // Define in constants or configuration const availSpaceInBytes = await getAvailableBackupSpaceInBytes( - "/appsmith-stacks", + backupPath, ); - checkAvailableBackupSpace(availSpaceInBytes); + checkAvailableBackupSpace(availSpaceInBytes, backupPath); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (11)
app/client/packages/rts/src/ctl/backup/BackupState.ts(1 hunks)app/client/packages/rts/src/ctl/backup/backup.test.ts(8 hunks)app/client/packages/rts/src/ctl/backup/index.ts(2 hunks)app/client/packages/rts/src/ctl/backup/links/BackupFolderLink.ts(1 hunks)app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts(2 hunks)app/client/packages/rts/src/ctl/backup/links/EncryptionLink.ts(2 hunks)app/client/packages/rts/src/ctl/backup/links/EnvFileLink.ts(1 hunks)app/client/packages/rts/src/ctl/backup/links/GitStorageLink.ts(1 hunks)app/client/packages/rts/src/ctl/backup/links/ManifestLink.ts(1 hunks)app/client/packages/rts/src/ctl/backup/links/MongoDumpLink.ts(1 hunks)app/client/packages/rts/src/ctl/backup/links/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/packages/rts/src/ctl/backup/links/ManifestLink.ts
🧰 Additional context used
📓 Learnings (2)
app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts (1)
Learnt from: sharat87
PR: appsmithorg/appsmith#37715
File: app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts:9-9
Timestamp: 2024-11-28T07:22:52.912Z
Learning: In the `DiskSpaceLink` class in `app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts`, the function `checkAvailableBackupSpace` already throws errors, so additional try-catch blocks are unnecessary.
app/client/packages/rts/src/ctl/backup/links/EncryptionLink.ts (2)
Learnt from: sharat87
PR: appsmithorg/appsmith#37715
File: app/client/packages/rts/src/ctl/backup/links/EncryptionLink.ts:10-17
Timestamp: 2024-11-28T07:25:52.356Z
Learning: The function `getEncryptionPasswordFromUser` in `EncryptionLink.ts` is not asynchronous.
Learnt from: sharat87
PR: appsmithorg/appsmith#37715
File: app/client/packages/rts/src/ctl/backup/links/EncryptionLink.ts:19-35
Timestamp: 2024-11-28T07:33:29.635Z
Learning: Errors in the `postBackup` method of the `EncryptionLink` class (`EncryptionLink.ts`) are already handled externally, so additional error handling within this method is unnecessary.
🔇 Additional comments (17)
app/client/packages/rts/src/ctl/backup/links/index.ts (2)
Line range hint 1-10: Well-designed interface with clear lifecycle hooks
The Link interface provides a clean contract for implementing backup operations with optional lifecycle hooks.
12-18: Verify module existence and consider type safety improvements
The export statements look organized. Let's verify all modules exist and consider adding explicit interface implementation checks.
Consider adding explicit type checks in each module:
class SomeLink implements Link {
async preBackup(): Promise<void> {
// Implementation
}
}✅ Verification successful
All modules exist and implement the Link interface correctly
The verification shows that:
- All 7 exported modules exist in the correct directory
- Each module properly implements the Link interface
- The implementations include necessary lifecycle methods (preBackup, doBackup, postBackup as needed)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify existence of all referenced modules and check interface implementation
# Check if all referenced modules exist
for module in BackupFolderLink DiskSpaceLink EncryptionLink EnvFileLink GitStorageLink ManifestLink MongoDumpLink; do
echo "Checking for $module..."
fd "${module}.ts" app/client/packages/rts/src/ctl/backup/links
done
# Check if modules implement Link interface
ast-grep --pattern 'class $_ implements Link {
$$$
}'
Length of output: 13892
app/client/packages/rts/src/ctl/backup/BackupState.ts (3)
2-2: LGTM! Strong immutability implementation.
The double readonly provides robust immutability protection for both the property and array.
12-18: LGTM! Robust immutability and encapsulation implementation.
Good use of Object.freeze() for args and Object.seal() for the instance. The comments effectively explain the reasoning behind using Object.seal().
9-9: Verify the impact of simplified encryption state.
The change from an encryption password to a boolean flag might affect encryption implementation elsewhere in the codebase.
✅ Verification successful
Change from encryption password to boolean flag is safe
The boolean flag isEncryptionEnabled is used correctly in conjunction with the actual encryption password which is:
- Handled internally in EncryptionLink using a private field
- Retrieved from environment variables in EnvFileLink
- Used properly in the encryption implementation with OpenSSL
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to encryption-related functionality
rg -l "encryptionPassword|isEncryptionEnabled" --type ts
# Look for encryption-related function implementations
ast-grep --pattern 'function $_($$encryption$$) { $$$ }'
Length of output: 348
Script:
#!/bin/bash
# Check the implementation details of encryption-related files
rg -A 10 "encryptionPassword|isEncryptionEnabled" app/client/packages/rts/src/ctl/backup/links/EncryptionLink.ts
rg -A 10 "encryptionPassword|isEncryptionEnabled" app/client/packages/rts/src/ctl/backup/links/EnvFileLink.ts
rg -A 10 "encryptionPassword|isEncryptionEnabled" app/client/packages/rts/src/ctl/backup/backup.test.ts
# Look for encryption-related class methods
ast-grep --pattern 'class $_ {
$$$
encrypt($$$) {
$$$
}
$$$
}'
Length of output: 2487
app/client/packages/rts/src/ctl/backup/links/MongoDumpLink.ts (1)
1-9: LGTM! Clean class structure with proper TypeScript practices.
Good use of TypeScript features with readonly and private modifiers.
app/client/packages/rts/src/ctl/backup/links/BackupFolderLink.ts (1)
1-6: LGTM! Clean import organization.
Imports are well-organized with types separated from runtime dependencies.
app/client/packages/rts/src/ctl/backup/links/GitStorageLink.ts (1)
30-37: Verify symbolic link creation across environments
The current implementation might face issues:
- Windows environments might require different commands
- Existing symbolic links at destination aren't handled
- Permission issues aren't handled explicitly
app/client/packages/rts/src/ctl/backup/links/EnvFileLink.ts (2)
1-11: LGTM! Clean imports and well-formatted warning message.
13-19: LGTM! Clean class implementation with proper documentation.
app/client/packages/rts/src/ctl/backup/backup.test.ts (4)
6-15: LGTM: Clean import restructuring
The imports have been properly organized to reflect the new link-based architecture.
70-70: LGTM: Comprehensive git root path testing
Good coverage of different scenarios for git root path resolution.
Also applies to: 74-74, 78-78
24-24: Verify disk space check implementation
The disk space validation tests look good, but let's verify the actual implementation.
Also applies to: 38-38, 43-43
✅ Verification successful
Disk space validation implementation looks correct
The implementation uses fsPromises.statfs to check available disk space and returns the product of block size (bsize) and free blocks (bfree), which is the standard way to calculate available space. The test validates against a 2GB minimum requirement.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check implementation of disk space validation
ast-grep --pattern 'function $_(path: string): number {
$$$
}'
Length of output: 66
Script:
#!/bin/bash
# Let's try a more specific search for the disk space validation function
rg -A 10 "getAvailableBackupSpaceInBytes" --type ts
Length of output: 3303
203-203: Security consideration: Password handling in tests
While the encryption tests are comprehensive, consider:
- Using a mock for password input instead of actual passwords in test data
- Adding test cases for password complexity requirements
Also applies to: 219-219, 225-225, 238-238
✅ Verification successful
Password handling in tests follows good practices
The test implementation already follows secure practices:
- Uses jest.fn() to mock the password input via readlineSync.question
- Test data passwords are only used within test scope
- Includes validation test case for password mismatch scenario
The current implementation is sufficient for testing purposes and doesn't expose sensitive information in test data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other password handling in codebase
rg -g '!*.test.ts' 'password.*=.*["\''][^\'"]+["\']'
Length of output: 116
Script:
#!/bin/bash
# Check for password handling in test file
rg -A 5 "getEncryptionPasswordFromUser" app/client/packages/rts/src/ctl/backup/backup.test.ts
# Check for password-related functions and mocks in test file
ast-grep --pattern 'function getEncryptionPasswordFromUser() { $$$ }'
# Check for password complexity validation
rg -g '*.ts' -g '*.js' "password.*validation|validatePassword"
Length of output: 819
Script:
#!/bin/bash
# Check the implementation of getEncryptionPasswordFromUser
rg -A 10 "function getEncryptionPasswordFromUser" app/client/packages/rts/src/ctl/backup/links.ts
# Look for password mocking in the test file
rg "jest.mock.*getEncryptionPasswordFromUser" app/client/packages/rts/src/ctl/backup/backup.test.ts
# Check if there's any password prompt implementation
rg "prompt.*password" app/client/packages/rts/src/ctl/backup/
Length of output: 672
Script:
#!/bin/bash
# Find the actual location of links.ts file
fd -e ts -e js links
# Check the test file content around password handling
rg -B 5 -A 5 "Test get encryption password from user prompt" app/client/packages/rts/src/ctl/backup/backup.test.ts
# Look for any mock implementations
rg -A 5 "jest.mock" app/client/packages/rts/src/ctl/backup/backup.test.ts
Length of output: 1129
app/client/packages/rts/src/ctl/backup/links/DiskSpaceLink.ts (1)
2-2: Ensure Constants.MIN_REQUIRED_DISK_SPACE_IN_BYTES is defined
Verify that MIN_REQUIRED_DISK_SPACE_IN_BYTES is correctly defined and exported from ../../constants.
app/client/packages/rts/src/ctl/backup/index.ts (2)
16-21: Ensure consistent use of state across Link instances
The DiskSpaceLink is instantiated without the state parameter, while other links receive state. Please verify if DiskSpaceLink doesn't require state or if it should be included for consistency.
23-25: Appropriate placement of EncryptionLink
Placing EncryptionLink at the end of the chain prevents unnecessary password prompts if preceding links fail.
The chain+link framework is now used for all the steps in the backup command. ## Automation /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/12080116829> > Commit: fcc016f > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12080116829&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Fri, 29 Nov 2024 07:12:22 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 ## Release Notes - **New Features** - Introduced new classes for managing backup processes, including `BackupFolderLink`, `DiskSpaceLink`, `EnvFileLink`, `GitStorageLink`, `MongoDumpLink`, and `ManifestLink`. - Added functionality for checking available disk space and managing temporary backup folders. - Enhanced encryption handling with a new password prompt mechanism. - **Bug Fixes** - Streamlined backup process by removing redundant functions and ensuring proper error handling. - **Documentation** - Updated comments and documentation for new classes and methods to improve clarity. - **Refactor** - Restructured backup functionality for improved modularity and maintainability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
The chain+link framework is now used for all the steps in the backup command.
Automation
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12080116829
Commit: fcc016f
Cypress dashboard.
Tags:
@tag.SanitySpec:
Fri, 29 Nov 2024 07:12:22 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
BackupFolderLink,DiskSpaceLink,EnvFileLink,GitStorageLink,MongoDumpLink, andManifestLink.Bug Fixes
Documentation
Refactor