chore: Add mongo data migration check before initializing the postgres dump#37059
chore: Add mongo data migration check before initializing the postgres dump#37059
Conversation
|
Warning Rate limit exceeded@abhvsn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 48 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
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
github-actions
bot
added
DB Infrastructure Pod
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
deploy/docker/fs/opt/appsmith/utils/bin/move-to-postgres.mjs (1)
21-24: Document the version mapping in comments.The constant
MINIMUM_MONGO_CHANGESETrepresents v1.43, but this mapping isn't clear from the code. Consider adding a comment that explicitly states this relationship.// The minimum version of the MongoDB changeset that must be present in the mongockChangeLog collection to run this script. // This is to ensure we are migrating the data from the stable version of MongoDB. +// This changeset corresponds to MongoDB version 1.43 const MINIMUM_MONGO_CHANGESET = "add_empty_policyMap_for_null_entries";
…ppsmithorg/appsmith into chore/add-version-check-for-mongo-data
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
deploy/docker/fs/opt/appsmith/utils/bin/move-to-postgres.mjs (1)
85-92: Consider adding success logging for version check.The error handling looks good, but adding a success log would improve observability.
if (!await isMongoDataMigratedToStableVersion(mongoDb)) { console.error("MongoDB migration check failed: Try upgrading the Appsmith instance to latest before opting for data migration."); console.error(`Could not find the valid migration execution entry for "${MINIMUM_MONGO_CHANGESET}" in the "${MONGO_MIGRATION_COLLECTION}" collection.`); await mongoClient.close(); mongoServer?.kill(); process.exit(1); +} else { + console.log("MongoDB version check passed. Proceeding with migration..."); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- deploy/docker/fs/opt/appsmith/utils/bin/move-to-postgres.mjs (3 hunks)
🔇 Additional comments (1)
deploy/docker/fs/opt/appsmith/utils/bin/move-to-postgres.mjs (1)
19-24: LGTM! Well-defined constants with clear purpose.The constants are appropriately named and documented, clearly indicating their role in version checking.
| const sortedCollectionNames = collectionNames.map(collection => collection.name).sort(); | ||
|
|
||
| // Verify that the MongoDB data has been migrated to a stable version i.e. v1.43 before we start migrating the data to Postgres. | ||
| if (!await isMongoDataMigratedToStableVersion(mongoDb)) { |
There was a problem hiding this comment.
Can you move this logic to the isMongoDataMigratedToStableVersion?
There was a problem hiding this comment.
Any reason for this ask? As the method suggests it only checks if the mongo data is migrated to correct changeId before starting the migration. Also if we go other route then instead of passing only the mongo client we will have to pass mongoServer as well which is not necessary.
There was a problem hiding this comment.
Cool works. Just wanted to keep all the changes related to version in a single method.
2 participants
Description
Fixes #36791
/test Sanity
🔍 Cypress test results
Warning
Tests have not run on the HEAD df02162 yet
Thu, 24 Oct 2024 10:10:48 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Bug Fixes
Chores