Updating the setup script to handle sample data import for Docker #2691
Conversation
WalkthroughThe pull request introduces enhancements to 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
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
![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 (1)
setup.ts (1)
1187-1213: Consider adding validation for npm command availability.The script assumes
npm run import:sample-datais available. Add validation to ensure the command exists.Consider:
- Validating package.json for the script definition
- Checking if npm is available in PATH
- Adding fallback mechanisms if the command fails
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
entrypoint.sh(1 hunks)setup.ts(1 hunks)
🧰 Additional context used
🪛 Shellcheck
entrypoint.sh
[error] 2-2: Remove leading spaces before the shebang.
(SC1114)
[error] 2-2: The shebang must be on the first line. Delete blanks and move comments.
(SC1128)
🔇 Additional comments (1)
setup.ts (1)
1187-1213: Verify Docker environment detection.
The Docker installation check relies solely on user input. Consider adding automated detection.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
setup.ts (1)
1187-1223: Consider refactoring Docker sample data import into a separate function.The implementation would be more maintainable if moved to a dedicated async function.
+async function importDockerSampleData(): Promise<void> { + console.log("Starting the sample data import for docker now..."); + + const entryPointScript = `#!/bin/bash +npm run import:sample-data +`; + + const scriptPath = path.join(os.tmpdir(), `entrypoint-${Date.now()}.sh`); + + try { + // Create script with proper permissions + fs.writeFileSync(scriptPath, entryPointScript, { mode: 0o755 }); + + await new Promise<void>((resolve, reject) => { + exec(scriptPath, { timeout: 60000 }, (error, stdout, stderr) => { + // Clean up script file + try { + fs.unlinkSync(scriptPath); + } catch (cleanupError) { + console.warn("Failed to cleanup script file:", cleanupError); + } + + if (error) { + console.error("Error importing sample data:"); + console.error(`Exit code: ${error.code}`); + console.error(`Error message: ${error.message}`); + reject(error); + return; + } + + if (stderr) { + console.warn("Sample data import warnings:"); + console.warn(stderr.trim()); + } + + if (stdout) { + console.log("Sample data import output:"); + console.log(stdout.trim()); + } + console.log("Sample data import complete."); + resolve(); + }); + }); + } catch (err) { + console.error("Failed to setup sample data import:", err); + if (fs.existsSync(scriptPath)) { + try { + fs.unlinkSync(scriptPath); + } catch (cleanupError) { + console.warn("Failed to cleanup script file:", cleanupError); + } + } + throw err; + } +} if (isDockerInstallation) { - console.log("Starting the sample data import for docker now..."); - ... + await importDockerSampleData(); }🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 1201-1201: Shell command built from environment values
This shell command depends on an uncontrolled absolute path.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
setup.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
setup.ts
[warning] 1201-1201: Shell command built from environment values
This shell command depends on an uncontrolled absolute path.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
setup.ts (1)
1184-1234: Implementation looks good with minor formatting suggestion.The Docker sample data import implementation follows security best practices:
- Uses secure temporary directory
- Includes comprehensive error handling
- Ensures proper cleanup of temporary files
- Sets appropriate file permissions
- Has execution timeout
Consider improving the script content formatting for better readability.
const entryPointScript = `#!/bin/bash -npm run import:sample-data + +# Import sample data for Docker environment +npm run import:sample-data + `;🧰 Tools
🪛 Biome
[error] 1197-1197: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1199-1199: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1199-1201: Illegal use of an import declaration not at the top level
move this declaration to the top level
(parse)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
setup.ts(1 hunks)
🧰 Additional context used
🪛 Biome
setup.ts
[error] 1197-1197: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1199-1199: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1199-1201: Illegal use of an import declaration not at the top level
move this declaration to the top level
(parse)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
setup.ts (1)
1184-1186: Improve code organization and remove redundant comment.The success message should be moved after the Docker check to avoid confusion, and the redundant comment can be removed.
console.log("\nCongratulations! Talawa API has been successfully setup! 🥂🎉"); -/* Performing the sample data import for docker */ if (isDockerInstallation) { + console.log("\nCongratulations! Talawa API has been successfully setup! 🥂🎉"); console.log("Starting the sample data import for docker now...");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
setup.ts(1 hunks)
🧰 Additional context used
🪛 Biome
setup.ts
[error] 1215-1215: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1215-1215: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1215-1215: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1216-1216: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1216-1216: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1216-1216: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1215-1215: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 1216-1216: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 1215-1216: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 1216-1217: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (1)
setup.ts (1)
1187-1237: LGTM! Implementation follows security best practices.
The Docker sample data import implementation demonstrates several good practices:
- Uses secure temporary directory with
os.tmpdir() - Includes comprehensive error handling
- Ensures cleanup of temporary files
- Sets appropriate file permissions (0o755)
- Has execution timeout (60s)
🧰 Tools
🪛 Biome
[error] 1215-1215: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1215-1215: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1215-1215: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1216-1216: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1216-1216: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1216-1216: Expected a semicolon or an implicit semicolon after a statement, but found none
An explicit or implicit semicolon is expected here...
...Which is required to end this statement
(parse)
[error] 1215-1215: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 1216-1216: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 1215-1216: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
[error] 1216-1217: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
|
@Suyash878 Isn't sample data is imported into docker mango container? |
|
Yes, sample data would be imported to the docker mongo container, totally depends on the mongodb url in the environment variables. |
|
Please fix the failing tests |
|
The base branch check is failing since it is not targeted to the postgres branch. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2691 +/- ##
========================================
Coverage 97.74% 97.74%
========================================
Files 358 358
Lines 18114 18114
Branches 2601 2601
========================================
Hits 17706 17706
Misses 404 404
Partials 4 4 ☔ View full report in Codecov by Sentry. |
palisadoes
added
the
ignore-sensitive-files-pr
|
Sure I will look into the changes. |
…ker and removing entrypoint.sh
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
.env.sample (3)
24-26: Improve the documentation for the DOCKER environment variable.The comments should be more specific about the expected values and usage. Consider enhancing the documentation:
-# This environment variable is for storing user's response to whether they are -# docker to setup or not. +# This environment variable indicates whether Docker is being used for the setup. +# Possible values: true/false +# Example: DOCKER=true
24-27: Consider grouping infrastructure-related environment variables together.The
DOCKERvariable would be better placed near other infrastructure or setup-related configurations. Consider moving it closer to the database configuration section since they're related to the deployment setup.
24-27: Consider adding more Docker-related configuration options.Given that this PR focuses on Docker integration, consider adding more Docker-related environment variables that might be needed for a complete setup:
# This environment variable indicates whether Docker is being used for the setup. # Possible values: true/false DOCKER= + +# Docker-specific configurations +# DOCKER_COMPOSE_FILE=docker-compose.yml +# DOCKER_API_VERSION= +# DOCKER_HOST= +# DOCKER_NETWORK=This would provide more flexibility and control over the Docker setup process.
setup.ts (1)
950-950: Simplify boolean expression.The ternary operator for the default value can be simplified.
- default: process.env.MONGO ? false : true, + default: !process.env.MONGO,🧰 Tools
🪛 Biome (1.9.4)
[error] 950-951: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
.env.sample(1 hunks)setup.ts(4 hunks)
🧰 Additional context used
🪛 eslint
setup.ts
[error] 9-9: 'child_process' imported multiple times.
(import/no-duplicates)
🪛 GitHub Check: Check for linting, formatting, and type errors
setup.ts
[failure] 9-9:
'child_process' imported multiple times
🪛 Biome (1.9.4)
setup.ts
[error] 950-951: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Please fix the failing tests |
|
The testing Application test is failing due to a bug I think, I've come across this a bunch of times and its something that needs fixing. |
|
The mongodb connection timed out. I'm rerunning the tests. |
|
|
For me it shows this as the failed test: |
|
I will look into it. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
setup.ts (4)
8-8: Consolidate child_process imports.Combine the imports from 'child_process' into a single import statement for better code organization.
-import { exec, spawn } from "child_process"; +import { exec, spawn, type ExecException } from "child_process";
489-510: Add timeout and cleanup handling for Docker operations.The Docker compose operation could potentially hang indefinitely. Consider adding:
- A timeout mechanism
- Cleanup handling for failed operations
return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + dockerCompose.kill(); + reject(new Error('Docker compose operation timed out after 5 minutes')); + }, 300000); + const dockerCompose = spawn( process.platform === "win32" ? "docker-compose.exe" : "docker-compose", ["-f", "docker-compose.dev.yaml", "up", "--build", "-d"], { stdio: "inherit" }, ); dockerCompose.on("error", (error) => { + clearTimeout(timeout); console.error("Error running docker-compose:", error); reject(error); }); dockerCompose.on("close", (code) => { + clearTimeout(timeout); if (code === 0) { console.log("Docker Compose completed successfully."); resolve(); } else { + // Attempt cleanup on failure + const cleanup = spawn( + process.platform === "win32" ? "docker-compose.exe" : "docker-compose", + ["-f", "docker-compose.dev.yaml", "down"], + { stdio: "inherit" } + ); reject(new Error(`Docker Compose exited with code ${code}`)); } }); });
967-967: Simplify the default value expression.The ternary operation with boolean literals can be simplified.
- default: process.env.MONGO ? false : true, + default: !process.env.MONGO,🧰 Tools
🪛 Biome (1.9.4)
[error] 967-968: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
1264-1267: Improve error handling in MongoDB connection check.The error variable is unused, and the error handling could be more informative.
- } catch (err) { + } catch (error) { + if (error instanceof Error) { + console.log(`MongoDB connection attempt failed: ${error.message}. Retrying in 1 second...`); + } await new Promise((resolve) => setTimeout(resolve, 1000)); }🧰 Tools
🪛 eslint
[error] 1264-1264: 'err' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Check for linting, formatting, and type errors
[failure] 1264-1264:
'err' is defined but never used
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
setup.ts(4 hunks)
🧰 Additional context used
🪛 GitHub Check: Check for linting, formatting, and type errors
setup.ts
[warning] 485-485:
Unexpected any. Specify a different type
[failure] 1264-1264:
'err' is defined but never used
🪛 Biome (1.9.4)
setup.ts
[error] 967-968: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🪛 eslint
setup.ts
[error] 1264-1264: 'err' is defined but never used.
(@typescript-eslint/no-unused-vars)
setup.ts
Outdated
| : reject(new Error("Docker daemon not running")), | ||
| ); | ||
| }); | ||
| } catch (error:any) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a more specific type for error handling.
Instead of using any, use unknown or Error type for better type safety.
-catch (error: any) {
+catch (error: unknown) {
if (error instanceof Error) {
throw new Error(`Docker daemon is not running. Please start Docker and try again. Details: ${error.message}`);
}
throw new Error('Docker daemon is not running. Please start Docker and try again.');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error:any) { | |
| } catch (error: unknown) { | |
| if (error instanceof Error) { | |
| throw new Error(`Docker daemon is not running. Please start Docker and try again. Details: ${error.message}`); | |
| } | |
| throw new Error('Docker daemon is not running. Please start Docker and try again.'); |
🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[warning] 485-485:
Unexpected any. Specify a different type
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
setup.ts (3)
469-512: Add timeout handling for Docker operations.The Docker compose operation could potentially hang indefinitely. Consider adding a timeout mechanism.
return new Promise((resolve, reject) => { + const timeout = setTimeout(() => { + dockerCompose.kill(); + reject(new Error('Docker compose operation timed out after 5 minutes')); + }, 300000); + const dockerCompose = spawn( process.platform === "win32" ? "docker-compose.exe" : "docker-compose", ["-f", "docker-compose.dev.yaml", "up", "--build", "-d"], { stdio: "inherit" } ); dockerCompose.on("error", (error) => { + clearTimeout(timeout); console.error("Error running docker-compose:", error); reject(error); }); dockerCompose.on("close", (code) => { + clearTimeout(timeout); if (code === 0) { console.log("Docker Compose completed successfully."); resolve(); } else { reject(new Error(`Docker Compose exited with code ${code}`)); } }); });🧰 Tools
🪛 GitHub Check: Check for linting, formatting, and type errors
[warning] 485-485:
Unexpected any. Specify a different type
969-969: Simplify the default value logic.The ternary expression for the default value can be simplified.
- default: process.env.MONGO ? false : true, + default: !process.env.MONGO,🧰 Tools
🪛 Biome (1.9.4)
[error] 969-970: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
1267-1270: Improve error message formatting.The error message concatenation can be improved for better readability.
- console.log( - "Waiting for MongoDB to be ready... Retrying in 1 second, Details:" + - err, - ); + console.log( + `Waiting for MongoDB to be ready... Retrying in 1 second. Details: ${err}` + );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
setup.ts(4 hunks)
🧰 Additional context used
🪛 GitHub Check: Check for linting, formatting, and type errors
setup.ts
[warning] 485-485:
Unexpected any. Specify a different type
🪛 Biome (1.9.4)
setup.ts
[error] 969-970: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (1)
setup.ts (1)
8-8: LGTM! Good import consolidation.
Combining the imports from 'child_process' into a single statement improves code organization.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
setup.ts (3)
485-489: Improve error message formatting.The error message concatenation could be improved for better readability and error details preservation.
- throw new Error( - `Docker daemon is not running. Please start Docker and try again. Details: ${error}`, - ); + if (error instanceof Error) { + throw new Error( + `Docker daemon is not running. Please start Docker and try again. Details: ${error.message}` + ); + } + throw new Error('Docker daemon is not running. Please start Docker and try again.');
969-969: Simplify boolean expression.The ternary operator for boolean values can be simplified.
- default: process.env.MONGO ? false : true, + default: !process.env.MONGO,🧰 Tools
🪛 Biome (1.9.4)
[error] 969-970: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
1278-1280: Improve error handling in Docker container startup.The error handling could be more informative and consistent with the rest of the codebase.
- } catch (err) { - console.log("Some error occurred: " + err); + } catch (err) { + const error = err instanceof Error ? err.message : String(err); + console.error(`Failed to start Docker containers: ${error}`); + throw err; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
setup.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
setup.ts
[error] 969-970: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
|
@Suyash878 Please fix the test so that this PR passes. This PR was supposed to have fixed the issue: |
|
@VanshikaSabharwal You have been working on Docker in Talawa Admin Can you provide any immediate assistance? This has reached a level of urgency. If so please open a PR against this issue as an emergency override of our policy. |
|
Yes @palisadoes i am doing it |
|
Please make code rabbit approve your changes |
|
@Suyash878 To approve code by code rabbit you have to make necessary changes recommended by code rabbit in you codebase and resolve each conversation |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
setup.ts (2)
976-976: Simplify the boolean expression.The ternary operator with boolean literals can be simplified.
- default: process.env.MONGO ? false : true, + default: !process.env.MONGO,🧰 Tools
🪛 Biome (1.9.4)
[error] 976-977: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
1292-1293: Improve error logging.The error concatenation could be improved with template literals and proper error type handling.
- console.log("Some error occurred: " + err); + const errorMessage = err instanceof Error ? err.message : String(err); + console.error(`Failed to start Docker containers: ${errorMessage}`);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
setup.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
setup.ts
[error] 976-977: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (3)
setup.ts (3)
8-8: LGTM! Clean import consolidation.
Good practice to consolidate multiple imports from the same module into a single import statement.
469-519: LGTM! Well-implemented Docker compose function.
The function includes several good practices:
- Cross-platform support
- Timeout handling
- Proper error handling
- Clear logging
1263-1286: LGTM! Well-implemented MongoDB connection retry logic.
The implementation includes several good practices:
- Maximum retry limit
- Clear error messages
- Proper connection testing with ping
- Appropriate delay between retries
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
setup.ts (1)
977-977: Simplify the boolean expression.The ternary operator for a boolean result can be simplified.
- default: process.env.MONGO ? false : true, + default: !process.env.MONGO,🧰 Tools
🪛 Biome (1.9.4)
[error] 977-978: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
setup.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
setup.ts
[error] 977-978: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (3)
setup.ts (3)
8-8: LGTM! Import statement is correctly placed.
The import of exec and spawn from 'child_process' is appropriate for the Docker-related functionality added.
469-520: Well-structured Docker setup function with good practices!
The function includes:
- Cross-platform support for Windows/Unix
- Proper error handling with type narrowing
- Timeout mechanism to prevent hanging
- Clear error messages
1264-1287: Well-implemented MongoDB readiness check!
The implementation includes:
- Proper timeout handling (30 seconds)
- Retry counter
- Clear error messages with retry status
- Type-safe error handling
| const dockerCompose = spawn( | ||
| process.platform === "win32" ? "docker-compose.exe" : "docker-compose", | ||
| ["-f", "docker-compose.dev.yaml", "up", "--build", "-d"], | ||
| { stdio: "inherit" }, | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using path.join for Docker compose file path.
For better cross-platform compatibility and to prevent path traversal attacks, use path.join for the Docker compose file path.
- ["-f", "docker-compose.dev.yaml", "up", "--build", "-d"],
+ ["-f", path.join(process.cwd(), "docker-compose.dev.yaml"), "up", "--build", "-d"],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const dockerCompose = spawn( | |
| process.platform === "win32" ? "docker-compose.exe" : "docker-compose", | |
| ["-f", "docker-compose.dev.yaml", "up", "--build", "-d"], | |
| { stdio: "inherit" }, | |
| ); | |
| const dockerCompose = spawn( | |
| process.platform === "win32" ? "docker-compose.exe" : "docker-compose", | |
| ["-f", path.join(process.cwd(), "docker-compose.dev.yaml"), "up", "--build", "-d"], | |
| { stdio: "inherit" }, | |
| ); |
| } catch (err) { | ||
| console.log("Some error occurred: " + err); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in catch block.
The error handling could be more informative and type-safe.
- console.log("Some error occurred: " + err);
+ const errorMessage = err instanceof Error ? err.message : String(err);
+ console.error(`Failed to start Docker containers: ${errorMessage}`);
+ throw err;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (err) { | |
| console.log("Some error occurred: " + err); | |
| } | |
| } catch (err) { | |
| const errorMessage = err instanceof Error ? err.message : String(err); | |
| console.error(`Failed to start Docker containers: ${errorMessage}`); | |
| throw err; | |
| } |
|
@palisadoes Shall I create a separate issue for the tests regarding these changes? Considering it would be a cleaner approach that way and since this discussion thread has already been quite cluttered with the main changes themselves. |
|
We cannot merge into the code base if there are errors. I'll close this until you figure out the solution. However we need this fixed urgently, so I'll open the creation of PRs for this issue to others. @VanshikaSabharwal had volunteered to fix it for example. |
What kind of change does this PR introduce?
Bug Fix
Issue Number:
Fixes #2270
Did you add tests for your changes?
No
Snapshots/Videos:
If relevant, did you update the documentation?
Not sure
Summary
The setup script will now prompt the user when it starts the process for sample data import provided the user is using Docker.
Does this PR introduce a breaking change?
No
Other information
This PR was supposed to be made against the
develop-postgresbranch but since it is currently under work, and unavailability of the setup script there I am currently raising the PR against thedevelopbranch.My changes may get introduced to the
develop-postgresbranch as a seperate issue later on.Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
DOCKERadded for enhanced configuration options.Bug Fixes