Updated setup.ts to support docker compose v2 and pass npm run typecheck #2770
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (5)
setup.ts (5)
8-8: Consider partial imports to reduce overhead.Importing multiple methods (exec, spawn, execSync) from child_process is fine, but if only a subset is needed for this file, consider a narrower import to optimize usage.
469-485: Double-check error logging for sensitive data.The catch block simply logs the entire error object using console.log(error). Depending on the environment, this may inadvertently reveal sensitive system or environment details. Consider using a more guarded logging approach.
} catch (error) { - console.log(error); + console.error("Error retrieving Docker Compose version:", error instanceof Error ? error.message : String(error));
501-505: Check platform compatibility for Docker daemon error messaging.While the error message references Docker Desktop, some platforms (e.g., Linux) may not have Docker Desktop but still run Docker daemon. Consider adjusting the messaging to provide platform-appropriate instructions.
520-529: Avoid abrupt termination without cleanup.Killing the Docker Compose process after the timeout helps prevent hanging. However, consider adding a more graceful cleanup or logs to inform the user about any incomplete jobs (e.g., partially built images).
532-534: Validate concurrency guard for multiple events.The isCompleted guard is used to prevent double-calling resolve or reject. This approach is correct, but be aware of potential race conditions if other event listeners also set isCompleted. Adding a small comment about why this guard exists could enhance maintainability.
Also applies to: 540-541
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
setup.ts(3 hunks)src/typeDefs/unions.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/typeDefs/unions.ts
|
@palisadoes Also, the main change which I have done is in setup.ts file to fix the bug. And as it is a sensitive file I have failed another test. How do I proceed further? |
|
@palisadoes I got why
The other 3 unit tests under "resolvers -> Query -> getVolunteerRanks" are correct, only this one had wrong hours written. I'm changing the getVolunteerRanks.spec.ts file in order to change the hours to 6 and for my PR to pass the TESTING APPLICATION(22.x) job. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/resolvers/Query/getVolunteerRanks.spec.ts (1)
80-80: Remove debug console.log statementDebug logging statements should not be committed to test files as they add noise to test output.
- console.log(volunteerRanks);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/resolvers/Query/getVolunteerRanks.spec.ts(1 hunks)
🔇 Additional comments (1)
tests/resolvers/Query/getVolunteerRanks.spec.ts (1)
81-81: Verify the volunteer hours calculation for monthly timeframe
The change from 2 to 6 hours appears correct based on PR comments, but let's verify the calculation logic.
✅ Verification successful
Based on the search results, I can now verify the volunteer hours calculation for the monthly timeframe. The code shows:
- In
tests/helpers/volunteers.ts, test data is created with hoursHistory entries at different dates - In
src/resolvers/Query/getVolunteerRanks.ts, the calculation logic filters hoursHistory based on timeframe - The test
getVolunteerRanks for monthlyintests/resolvers/Query/getVolunteerRanks.spec.tsexpects 6 hours, which matches the sum of hours within a month's timeframe from the test data
The 6 hours value is correct for monthly timeframe
The value matches the sum of hours from hoursHistory entries within the monthly period (2 hours from yesterday + 4 hours from two weeks ago = 6 hours total).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the volunteer hours calculation logic
echo "Searching for volunteer hours calculation..."
rg -A 10 "hoursVolunteered|createVolunteerAndActions" --type ts
# Check the helper function implementation
echo "Checking volunteer creation helper..."
ast-grep --pattern 'export const createVolunteerAndActions = async () => {
$$$
}'
Length of output: 42302
|
Tip For best results, initiate chat on the files or code changes. @palisadoes: How can I help you today? Let me know if you have any further instructions or questions about this PR. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Please fix:
Make sure CodeRabbit.ai approves your changes |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
setup.ts (3)
8-8: Consider product-level alternatives for Docker/child process management.
While importing "exec", "spawn", and "execSync" from "child_process" may be sufficient for now, consider using higher-level automation tools (e.g., node’s built-inchild_processwith detailed logging) or libraries (like execa) for better error handling.
469-485: Ensure compatibility checks for Docker Compose are robust.
This function gracefully handles the Docker Compose v1 vs. v2 distinction by trying v2 first and falling back to v1. However, some edge cases might appear if Docker is installed without “docker compose” support or if “docker-compose” is absent. Consider verifying if the user has Docker installed at all and giving a more descriptive message before attempting the version check.
501-505: Validate concurrency handling in process timeout logic.
In "runDockerComposeWithLogs", you handle a 5-minute timeout and kill the Docker Compose process if it’s still running. This is good for preventing hangs, but be mindful of concurrency scenarios where multiple scripts or different processes might also attempt to access Docker. Consider building in a retry mechanism or separate checks for conditions that might trigger transient Docker issues.Also applies to: 511-515, 520-529, 532-533, 540-541
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
setup.ts(3 hunks)src/typeDefs/unions.ts(1 hunks)tests/resolvers/Query/getVolunteerRanks.spec.ts(1 hunks)
🔇 Additional comments (2)
src/typeDefs/unions.ts (1)
1-4: Removing unused imports and exports is a good practice.
Commenting out the unused gql import and the unions export helps avoid confusion, reduces dead code, and speeds up build processes.
tests/resolvers/Query/getVolunteerRanks.spec.ts (1)
80-80: Confirm updated volunteer hours with data source.
The test expectation changes from 2 to 6 hours. This likely reflects a corrected fixture or updated data. Be sure the underlying logic or seed data is consistent with this revised expectation to avoid confusion in future merges.
palisadoes
merged commit 0ca595e
into
PalisadoesFoundation:develop
What kind of change does this PR introduce?
Changes in setup.ts, src/typedefs/unions.ts
Now the setup.ts is compatible with both docker compose v1 and v2, and the error handling of the
The empty gql which was creating EOF error during npm run typecheck has been removed. Moreover, the module unions.ts exported wasn't being used anywhere in the codebase.
This PR also fixes a unit test which expected wrong volunteered hours(details in the comment below).
Issue Number:
Fixes #2767, #2769
Did you add tests for your changes?
No
Snapshots/Videos:
If relevant, did you update the documentation?
Updation in the documentation is not required.
Summary
Now the docker containers start without facing any error related to outdated Docker Compose version. Also the error faced in the test
npm run typecheckis now fixed by removing the empty gql block.Does this PR introduce a breaking change?
No, the bug only effected
npm run setupandnpm run typecheckcommands.Have you read the contributing guide?
Yes
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores