chore: Format appsmithctl code #37532
Conversation
WalkthroughThe changes in this pull request primarily focus on formatting and consistency improvements across various JavaScript files related to backup and database operations. Key modifications include standardizing string quotation marks from single quotes to double quotes, enhancing readability through consistent indentation and line breaks, and adding semicolons where necessary. Additionally, several files have undergone minor structural adjustments to maintain clarity without altering the core functionality or control flow. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
🪧 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
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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 (10)
deploy/docker/fs/opt/appsmith/utils/bin/utils.js (3)
42-61: Add input validation for environment variablesWhile the logic is sound, consider adding validation for malformed environment variables:
let dbUrl = ""; try { let env_array = fs .readFileSync(Constants.ENV_PATH, "utf8") .toString() .split("\n"); for (let i in env_array) { if ( env_array[i].startsWith("APPSMITH_MONGODB_URI") || env_array[i].startsWith("APPSMITH_DB_URL") ) { - dbUrl = env_array[i].toString().split("=")[1].trim(); + const parts = env_array[i].toString().split("="); + if (parts.length === 2) { + dbUrl = parts[1].trim(); + } break; } } } catch (err) { console.error("Error reading the environment file:", err); }
121-125: Use template literals for better readabilityConsider using template literals for string concatenation:
- const output = ( - outChunks.join("").trim() + - "\n" + - errChunks.join("").trim() - ).trim(); + const output = `${outChunks.join("").trim()}\n${errChunks.join("").trim()}`.trim();
168-171: Add error handling for JSON parsingConsider adding try-catch for JSON parsing:
async function getCurrentAppsmithVersion() { + try { return ( JSON.parse(await fsPromises.readFile("/opt/appsmith/info.json", "utf8")) .version ?? "" ); + } catch (error) { + console.error("Error reading version info:", error); + return ""; + } }deploy/docker/fs/opt/appsmith/utils/bin/move-to-postgres.mjs (1)
50-58: Consider making the temporary directory path configurable.The hardcoded path
/tmp/db-tmpmight not be suitable for all environments. Consider making this configurable through an environment variable.+const TMP_DB_PATH = process.env.MONGO_TMP_PATH || "/tmp/db-tmp"; - fs.mkdirSync("/tmp/db-tmp", { recursive: true }); + fs.mkdirSync(TMP_DB_PATH, { recursive: true }); mongoServer = spawn( "mongod", - ["--bind_ip_all", "--dbpath", "/tmp/db-tmp", "--port", "27500"], + ["--bind_ip_all", "--dbpath", TMP_DB_PATH, "--port", "27500"],Also applies to: 63-68
deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js (6)
21-21: Fix typo in test descriptionThe test description contains a typo: "Checkx" should be "Checks".
- it("Checkx the constant is 2 GB", () => { + it("Checks the constant is 2 GB", () => {
31-31: Fix duplicate word in test descriptionThe test description contains a duplicate word: "hould".
- 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", () => {
41-41: Complete the truncated test descriptionThe test description is incomplete: "Generates t".
- it("Generates t", async () => { + it("Generates temporary backup directory path", async () => {
64-64: Remove debug console.log statementsRemove unnecessary console.log statements from test cases. These should not be part of the final test code.
Also applies to: 86-86, 133-133, 144-144, 155-155, 166-166, 177-177, 187-187, 197-197, 250-250
255-284: Group MongoDB URI parsing tests in a describe blockConsider grouping these related tests under a describe block for better organization.
+describe("MongoDB URI parsing", () => { test("Get DB name from Mongo URI 1", async () => { // ... existing test code }); // ... other MongoDB URI tests +});
8-284: Consider improving test descriptions for clarityWhile the test coverage is comprehensive, consider making test descriptions more descriptive of what they're actually testing. For example:
- "Backup Archive Limit when env APPSMITH_BACKUP_ARCHIVE_LIMIT is null" could be "Should return default limit (4) when APPSMITH_BACKUP_ARCHIVE_LIMIT is not set"
- "Test backup encryption function" could be "Should correctly encrypt backup archive with provided password"
This makes it easier to understand test failures and maintain the test suite.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (14)
deploy/docker/fs/opt/appsmith/utils/bin/backup.js(5 hunks)deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js(1 hunks)deploy/docker/fs/opt/appsmith/utils/bin/check_replica_set.js(2 hunks)deploy/docker/fs/opt/appsmith/utils/bin/constants.js(1 hunks)deploy/docker/fs/opt/appsmith/utils/bin/export_db.js(2 hunks)deploy/docker/fs/opt/appsmith/utils/bin/import_db.js(3 hunks)deploy/docker/fs/opt/appsmith/utils/bin/logger.js(3 hunks)deploy/docker/fs/opt/appsmith/utils/bin/mailer.js(2 hunks)deploy/docker/fs/opt/appsmith/utils/bin/mongo_shell_utils.js(2 hunks)deploy/docker/fs/opt/appsmith/utils/bin/move-to-postgres.mjs(6 hunks)deploy/docker/fs/opt/appsmith/utils/bin/restore.js(3 hunks)deploy/docker/fs/opt/appsmith/utils/bin/utils.js(7 hunks)deploy/docker/fs/opt/appsmith/utils/bin/utils.test.js(2 hunks)deploy/docker/fs/opt/appsmith/utils/bin/version.js(2 hunks)
✅ Files skipped from review due to trivial changes (9)
- deploy/docker/fs/opt/appsmith/utils/bin/backup.js
- deploy/docker/fs/opt/appsmith/utils/bin/check_replica_set.js
- deploy/docker/fs/opt/appsmith/utils/bin/constants.js
- deploy/docker/fs/opt/appsmith/utils/bin/export_db.js
- deploy/docker/fs/opt/appsmith/utils/bin/logger.js
- deploy/docker/fs/opt/appsmith/utils/bin/mailer.js
- deploy/docker/fs/opt/appsmith/utils/bin/mongo_shell_utils.js
- deploy/docker/fs/opt/appsmith/utils/bin/utils.test.js
- deploy/docker/fs/opt/appsmith/utils/bin/version.js
🧰 Additional context used
📓 Learnings (1)
deploy/docker/fs/opt/appsmith/utils/bin/move-to-postgres.mjs (2)
Learnt from: abhvsn
PR: appsmithorg/appsmith#36556
File: deploy/docker/fs/opt/appsmith/utils/bin/move-to-postgres.mjs:159-164
Timestamp: 2024-11-12T08:11:36.416Z
Learning: When transforming the `_class` field in `move-to-postgres.mjs`, if `mapClassToType` returns `null`, retain the original `_class` value in the `type` field to preserve important MongoDB information.
Learnt from: abhvsn
PR: appsmithorg/appsmith#36556
File: deploy/docker/fs/opt/appsmith/utils/bin/move-to-postgres.mjs:159-164
Timestamp: 2024-11-12T08:11:43.278Z
Learning: When transforming the `_class` field in `move-to-postgres.mjs`, if `mapClassToType` returns `null`, retain the original `_class` value in the `type` field to preserve important MongoDB information.
🪛 Biome
deploy/docker/fs/opt/appsmith/utils/bin/import_db.js
[error] 60-60: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
deploy/docker/fs/opt/appsmith/utils/bin/move-to-postgres.mjs
[error] 197-197: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
deploy/docker/fs/opt/appsmith/utils/bin/restore.js
[error] 150-150: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 42-42: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
🔇 Additional comments (6)
deploy/docker/fs/opt/appsmith/utils/bin/utils.js (2)
4-4: Good use of the node: prefix for fs module
Using the node: prefix is a security best practice that prevents dependency confusion attacks.
180-200: Well-structured MongoDB connection handling
Good implementation of MongoDB connection string preprocessing with proper handling of different scenarios and optimizations for localhost connections.
deploy/docker/fs/opt/appsmith/utils/bin/move-to-postgres.mjs (4)
9-10: LGTM: Import statements and warning message formatting.
The changes maintain code consistency while preserving the original functionality.
Also applies to: 34-36
169-176: LGTM: Object key sorting implementation.
The replacer function maintains a consistent order of keys, which is crucial for reproducible migrations.
195-197: Retain delete operator for data transformation.
While static analysis flags the delete operator for performance concerns, its use here is appropriate for data transformation. The alternative of setting undefined would leave remnants in the JSON output.
Note: This aligns with the previous learning about preserving MongoDB information through proper type field handling.
🧰 Tools
🪛 Biome
[error] 197-197: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
93-107: Verify migration version check implementation.
The migration version check is a critical safeguard. Let's verify its implementation across the codebase.
Also applies to: 119-127
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
deploy/docker/fs/opt/appsmith/utils/bin/backup.test.js (5)
21-24: Fix typo in test descriptionChange "Checkx" to "Checks" in the test description.
- it("Checkx the constant is 2 GB", () => { + it("Checks the constant is 2 GB", () => {
31-39: Fix typo in test descriptionRemove the duplicate "hould" in the test description.
- 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", () => {
41-46: Complete the test descriptionThe test description is incomplete. It should describe what is being generated.
- it("Generates t", async () => { + it("Generates temporary backup directory path", async () => {
245-274: Refactor MongoDB URI tests using test.eachConsider refactoring these similar test cases using Jest's test.each for better maintainability.
+const mongoUriTestCases = [ + { + uri: "mongodb+srv://admin:[email protected]/my_db_name?retryWrites=true&minPoolSize=1&maxPoolSize=10&maxIdleTimeMS=900000&authSource=admin", + expected: "my_db_name" + }, + { + uri: "mongodb+srv://admin:[email protected]/test123?retryWrites=true&minPoolSize=1&maxPoolSize=10&maxIdleTimeMS=900000&authSource=admin", + expected: "test123" + }, + { + uri: "mongodb+srv://admin:[email protected]/test123", + expected: "test123" + }, + { + uri: "mongodb://appsmith:pAssW0rd!@localhost:27017/appsmith", + expected: "appsmith" + } +]; + +test.each(mongoUriTestCases)("Get DB name from Mongo URI: $expected", ({ uri, expected }) => { + const dbName = utils.getDatabaseNameFromMongoURI(uri); + expect(dbName).toEqual(expected); +});
10-10: Remove unnecessary console.log statementsTest cases should not contain debug console.log statements unless they serve a specific purpose. Consider removing them to keep the test output clean.
Also applies to: 64-64, 86-86, 133-133, 144-144, 155-155, 166-166, 177-177, 187-187, 240-240
sharat87
requested review from
mohanarpit,
trishaanand,
abhvsn,
riodeuno,
AnaghHegde and
nidhi-nair
Formatting just before merging
appsmithctlcode into RTS.Automation
/test sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11890257914
Commit: 5c83801
Cypress dashboard.
Tags:
@tag.SanitySpec:
Mon, 18 Nov 2024 10:46:20 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Refactor
Tests