-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Restore doesn't work when backup file is renamed #37666
Conversation
WalkthroughThe pull request introduces modifications to 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
|
/build-deploy-preview skip-tests=true |
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12003516564. |
Deploy-Preview-URL: https://ce-37666.dp.appsmith.com |
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/packages/rts/src/ctl/restore.ts (2)
273-275
: Consider using template literals for better readabilityThe message construction could be more readable using template literals.
- "Please update your appsmith image to 'index.docker.io/appsmith/appsmith-ce:" + - restoreVersion + - "' in the 'docker-compose.yml' file\nand run the cmd: 'docker-compose restart' " + - "after the restore process is completed, to ensure the restored instance runs successfully.", + `Please update your appsmith image to 'index.docker.io/appsmith/appsmith-ce:${restoreVersion}' in the 'docker-compose.yml' file\nand run the cmd: 'docker-compose restart' after the restore process is completed, to ensure the restored instance runs successfully.`,
394-418
: Consider early return pattern for cleaner code flowThe function handles the backup discovery well, but could be more concise using early returns.
async function figureOutContentsPath(root: string): Promise<string> { const subfolders = await fsPromises.readdir(root, { withFileTypes: true }); - for (const subfolder of subfolders) { - if (subfolder.isDirectory()) { - try { - // Try to find the `manifest.json` file. - await fsPromises.access( - path.join(root, subfolder.name, "manifest.json"), - ); - - return path.join(root, subfolder.name); - } catch (error) { - // If that fails, look for the MongoDB data archive, since some very old backups won't have `manifest.json`. - await fsPromises.access( - path.join(root, subfolder.name, "mongodb-data.gz"), - ); - - return path.join(root, subfolder.name); - } - } - } + const directories = subfolders.filter(subfolder => subfolder.isDirectory()); + + for (const dir of directories) { + const dirPath = path.join(root, dir.name); + + // Try manifest.json first + try { + await fsPromises.access(path.join(dirPath, "manifest.json")); + return dirPath; + } catch {} // Intentionally empty - try mongodb-data.gz next + + // Try mongodb-data.gz for older backups + try { + await fsPromises.access(path.join(dirPath, "mongodb-data.gz")); + return dirPath; + } catch {} // Not found in this directory + } throw new Error("Could not find the contents of the backup archive."); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/packages/rts/src/ctl/backup.ts
(0 hunks)app/client/packages/rts/src/ctl/restore.ts
(4 hunks)
💤 Files with no reviewable changes (1)
- app/client/packages/rts/src/ctl/backup.ts
🔇 Additional comments (2)
app/client/packages/rts/src/ctl/restore.ts (2)
252-253
: LGTM! Improved cross-platform path handling
Using path.join
instead of string concatenation ensures correct path separators across different operating systems.
355-355
: LGTM! Good integration of the new backup discovery logic
The new figureOutContentsPath
function is properly integrated into the restore flow, with errors properly propagated to the caller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/packages/rts/src/ctl/restore.ts (2)
273-275
: Consider using template literals for better readabilityThe string concatenation can be simplified using template literals.
- "Please update your appsmith image to 'index.docker.io/appsmith/appsmith-ce:" + - restoreVersion + - "' in the 'docker-compose.yml' file\nand run the cmd: 'docker-compose restart' " + - "after the restore process is completed, to ensure the restored instance runs successfully.", + `Please update your appsmith image to 'index.docker.io/appsmith/appsmith-ce:${restoreVersion}' in the 'docker-compose.yml' file\nand run the cmd: 'docker-compose restart' after the restore process is completed, to ensure the restored instance runs successfully.`,
396-435
: Good implementation with room for minor improvementsThe function effectively handles both new and legacy backup formats. Consider these refinements:
async function figureOutContentsPath(root: string): Promise<string> { const subfolders = await fsPromises.readdir(root, { withFileTypes: true }); + // Check root first try { - // Check if the root itself contains the contents. await fsPromises.access(path.join(root, "manifest.json")); return root; } catch (error) { - // Ignore + // Root doesn't contain manifest.json, continue to check subfolders } - for (const subfolder of subfolders) { + for (const subfolder of subfolders as fsPromises.Dirent[]) { if (subfolder.isDirectory()) { + const subfolderPath = path.join(root, subfolder.name); + try { - // Try to find the `manifest.json` file. - await fsPromises.access( - path.join(root, subfolder.name, "manifest.json"), - ); - - return path.join(root, subfolder.name); + await fsPromises.access(path.join(subfolderPath, "manifest.json")); + return subfolderPath; } catch (error) { - // Ignore + // Try legacy format } try { - // If that fails, look for the MongoDB data archive, since backups from v1.7.x and older won't have `manifest.json`. - await fsPromises.access( - path.join(root, subfolder.name, "mongodb-data.gz"), - ); - - return path.join(root, subfolder.name); + await fsPromises.access(path.join(subfolderPath, "mongodb-data.gz")); + return subfolderPath; } catch (error) { - // Ignore + // Neither manifest.json nor mongodb-data.gz found in this subfolder } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/packages/rts/src/ctl/restore.ts
(4 hunks)
🔇 Additional comments (2)
app/client/packages/rts/src/ctl/restore.ts (2)
252-253
: LGTM: Improved path handling
Using path.join
is the correct approach for cross-platform compatibility.
357-359
: LGTM: Clean integration of new function
The integration is clean and maintains good code readability with appropriate spacing.
) When backups are renamed from their original generated name, restore operation can't process that backup anymore. This PR should fix that. ## 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/12004342912> > Commit: ef62c35 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12004342912&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity` > Spec: > <hr>Mon, 25 Nov 2024 06:54:45 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 - **New Features** - Introduced a new function to determine the contents path within backup archives, enhancing backup restoration capabilities. - **Bug Fixes** - Improved error handling for invalid backup file indices and password attempts during restoration. - **Refactor** - Streamlined output and error handling in the backup process, including cleanup and email notifications for backup errors. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
When backups are renamed from their original generated name, restore operation can't process that backup anymore. This PR should fix that.
Automation
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12004342912
Commit: ef62c35
Cypress dashboard.
Tags:
@tag.Sanity
Spec:
Mon, 25 Nov 2024 06:54:45 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Refactor