Updating the setup script to handle sample data import for Docker #2691
Conversation
WalkthroughA new shell script file Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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 ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. 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 2599 2599
========================================
Hits 17706 17706
Misses 404 404
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
palisadoes
added
the
ignore-sensitive-files-pr
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
entrypoint.sh) for importing sample data.entrypoint.shscript for importing sample data, with improved error handling.Bug Fixes