fixed pr workflow which ensures that talawa Api app starts in docker #2653
Conversation
WalkthroughThe changes in this pull request enhance the GitHub Actions workflow defined in 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Congratulations on making your first PR! 🎊 If you haven't already, check out our Contributing Guidelines and PR Reporting Guidelines to ensure that you are following our guidelines for contributing and creating PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
.github/workflows/pull-request.yml
Outdated
|
|
||
| - name: Checks Talawa API app starts in Docker | ||
| run: | | ||
| if [[ "${{ runner.os }}" == "Linux" ]]; then | ||
| bash sudo /usr/libexec/docker/cli-plugins/docker-compose -f docker-compose.dev.yaml up --build | ||
| elif [[ "${{ runner.os }}" == "Windows" ]]; then | ||
| docker-compose -f docker-compose.dev.yaml up --build | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve workflow structure to avoid resource conflicts
The current workflow runs both Docker containers and local development server in the same job, which could lead to port conflicts and redundant testing. Consider restructuring the workflow for better separation of concerns.
Suggestions:
- Split into separate jobs:
Test-Application: Run tests locally (existing)Docker-Check: Run Docker validation (new)
- Add job dependency:
Docker-Check: needs: [Test-Application] # ... Docker check implementation
This separation will:
- Prevent port conflicts
- Make the workflow more maintainable
- Provide clearer failure points
- Allow parallel execution if needed
.github/workflows/pull-request.yml
Outdated
| - name: Checks Talawa API app starts in Docker | ||
| run: | | ||
| if [[ "${{ runner.os }}" == "Linux" ]]; then | ||
| bash sudo /usr/libexec/docker/cli-plugins/docker-compose -f docker-compose.dev.yaml up --build | ||
| elif [[ "${{ runner.os }}" == "Windows" ]]; then | ||
| docker-compose -f docker-compose.dev.yaml up --build | ||
| fi |
There was a problem hiding this comment.
Critical issues in Docker startup check implementation
The current implementation has several issues that need to be addressed:
- The Docker Compose command path is incorrect and non-standard
- Missing detached mode will cause the workflow to hang
- No health check to verify successful startup
- No timeout handling
- No cleanup of Docker resources
- Unnecessary Windows condition (GitHub Actions uses Ubuntu runners)
Replace the implementation with this more robust version:
- - name: Checks Talawa API app starts in Docker
- run: |
- if [[ "${{ runner.os }}" == "Linux" ]]; then
- bash sudo /usr/libexec/docker/cli-plugins/docker-compose -f docker-compose.dev.yaml up --build
- elif [[ "${{ runner.os }}" == "Windows" ]]; then
- docker-compose -f docker-compose.dev.yaml up --build
- fi
+ - name: Check if Talawa API starts in Docker
+ run: |
+ # Start containers in detached mode
+ docker-compose -f docker-compose.dev.yaml up -d --build
+
+ # Wait for the API to be healthy (max 60 seconds)
+ timeout=60
+ until curl -f http://localhost:4000/health || [ $timeout -eq 0 ]; do
+ echo "Waiting for API to start... ($timeout seconds remaining)"
+ sleep 1
+ ((timeout--))
+ done
+
+ if [ $timeout -eq 0 ]; then
+ echo "Error: API failed to start within timeout"
+ docker-compose -f docker-compose.dev.yaml logs
+ exit 1
+ fi
+
+ echo "API started successfully"
+
+ # Cleanup
+ docker-compose -f docker-compose.dev.yaml down -vThis implementation:
- Uses standard
docker-composecommand - Runs containers in detached mode
- Implements proper health check with timeout
- Shows container logs on failure
- Cleans up resources after check
📝 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.
| - name: Checks Talawa API app starts in Docker | |
| run: | | |
| if [[ "${{ runner.os }}" == "Linux" ]]; then | |
| bash sudo /usr/libexec/docker/cli-plugins/docker-compose -f docker-compose.dev.yaml up --build | |
| elif [[ "${{ runner.os }}" == "Windows" ]]; then | |
| docker-compose -f docker-compose.dev.yaml up --build | |
| fi | |
| - name: Check if Talawa API starts in Docker | |
| run: | | |
| # Start containers in detached mode | |
| docker-compose -f docker-compose.dev.yaml up -d --build | |
| # Wait for the API to be healthy (max 60 seconds) | |
| timeout=60 | |
| until curl -f http://localhost:4000/health || [ $timeout -eq 0 ]; do | |
| echo "Waiting for API to start... ($timeout seconds remaining)" | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: API failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs | |
| exit 1 | |
| fi | |
| echo "API started successfully" | |
| # Cleanup | |
| docker-compose -f docker-compose.dev.yaml down -v |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/pull-request.yml
299-299: "runs-on" section is missing in job "Docker-Check"
(syntax-check)
🪛 yamllint
.github/workflows/pull-request.yml
[error] 298-298: trailing spaces
(trailing-spaces)
[error] 301-301: trailing spaces
(trailing-spaces)
[error] 305-305: trailing spaces
(trailing-spaces)
[error] 312-312: trailing spaces
(trailing-spaces)
[error] 318-318: trailing spaces
(trailing-spaces)
[error] 320-320: trailing spaces
(trailing-spaces)
.github/workflows/pull-request.yml
Outdated
| - name: Check if Talawa API starts in Docker | ||
| run: | | ||
| docker-compose -f docker-compose.dev.yaml up -d --build | ||
|
|
||
| timeout=60 | ||
| until curl -f http://localhost:4000/health || [ $timeout -eq 0 ]; do | ||
| echo "Waiting for API to start... ($timeout seconds remaining)" | ||
| sleep 1 | ||
| ((timeout--)) | ||
| done | ||
|
|
||
| if [ $timeout -eq 0 ]; then | ||
| echo "Error: API failed to start within timeout" | ||
| docker-compose -f docker-compose.dev.yaml logs | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "API started successfully" | ||
|
|
||
| # Cleanup | ||
| docker-compose -f docker-compose.dev.yaml down -v |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Docker check reliability
While the implementation is good, there are a few improvements needed:
- Add error handling for docker-compose commands
- Use environment variable for health check URL
- Ensure no port conflicts with Test-Application job
Apply this diff to enhance the implementation:
+ name: Check if Talawa API starts in Docker
run: |
+ # Ensure no containers are running
+ docker-compose -f docker-compose.dev.yaml down -v || true
+
+ # Start containers
docker-compose -f docker-compose.dev.yaml up -d --build
+ if [ $? -ne 0 ]; then
+ echo "Failed to start containers"
+ exit 1
+ fi
timeout=60
- until curl -f http://localhost:4000/health || [ $timeout -eq 0 ]; do
+ until curl -f "${HEALTH_CHECK_URL:-http://localhost:4000}/health" || [ $timeout -eq 0 ]; do
echo "Waiting for API to start... ($timeout seconds remaining)"
sleep 1
((timeout--))
done
if [ $timeout -eq 0 ]; then
echo "Error: API failed to start within timeout"
docker-compose -f docker-compose.dev.yaml logs
+ docker-compose -f docker-compose.dev.yaml down -v
exit 1
fi
echo "API started successfully"
# Cleanup
docker-compose -f docker-compose.dev.yaml down -v
+ if [ $? -ne 0 ]; then
+ echo "Failed to cleanup containers"
+ exit 1
+ fiAdditionally, consider adding these environment variables to the job:
env:
HEALTH_CHECK_URL: http://localhost:4000
COMPOSE_PROJECT_NAME: pr-${{ github.event.pull_request.number }}The COMPOSE_PROJECT_NAME ensures unique container names per PR, preventing conflicts with other jobs.
🧰 Tools
🪛 yamllint
[error] 305-305: trailing spaces
(trailing-spaces)
[error] 312-312: trailing spaces
(trailing-spaces)
[error] 318-318: trailing spaces
(trailing-spaces)
[error] 320-320: trailing spaces
(trailing-spaces)
.github/workflows/pull-request.yml
Outdated
| Docker-Check: | ||
| needs: Test-Application | ||
| steps: |
There was a problem hiding this comment.
Add missing required "runs-on" field
The job definition is missing the required "runs-on" field which specifies the runner environment. This will cause the workflow to fail.
Apply this diff to fix the job definition:
Docker-Check:
needs: Test-Application
+ runs-on: ubuntu-latest
steps:📝 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.
| Docker-Check: | |
| needs: Test-Application | |
| steps: | |
| Docker-Check: | |
| needs: Test-Application | |
| runs-on: ubuntu-latest | |
| steps: |
🧰 Tools
🪛 actionlint
299-299: "runs-on" section is missing in job "Docker-Check"
(syntax-check)
🪛 yamllint
[error] 301-301: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/pull-request.yml
304-304: shellcheck reported issue in this script: SC2181:style:6:7: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
304-304: shellcheck reported issue in this script: SC2181:style:29:7: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
🪛 yamllint
.github/workflows/pull-request.yml
[error] 298-298: trailing spaces
(trailing-spaces)
[error] 302-302: trailing spaces
(trailing-spaces)
[error] 320-320: trailing spaces
(trailing-spaces)
[error] 339-339: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
299-340: Verify workflow execution order and failure handling
The job integration looks good, but let's verify the workflow execution order and failure handling.
✅ Verification successful
Workflow execution order and failure handling are correctly implemented
Docker-Checkjob properly depends onTest-Application.- Cleanup steps are executed in case of failure.
- Proper error handling with
exit 1is in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify workflow execution order and failure handling
# Expected: Docker-Check job should run after Test-Application and handle failures correctly
# Check if Docker-Check depends on Test-Application
rg -A 5 "Docker-Check:" .github/workflows/pull-request.yml
# Check if cleanup is performed in case of failure
rg -A 10 "docker-compose.*down" .github/workflows/pull-request.yml
# Check for proper error handling
rg "exit 1" .github/workflows/pull-request.yml
Length of output: 1357
🧰 Tools
🪛 actionlint
304-304: shellcheck reported issue in this script: SC2181:style:6:7: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
304-304: shellcheck reported issue in this script: SC2181:style:29:7: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
🪛 yamllint
[error] 302-302: trailing spaces
(trailing-spaces)
[error] 320-320: trailing spaces
(trailing-spaces)
[error] 339-339: trailing spaces
(trailing-spaces)
.github/workflows/pull-request.yml
Outdated
| - name: Check if Talawa API starts in Docker | ||
| run: | | ||
| # Ensure no containers are running | ||
| docker-compose -f docker-compose.dev.yaml down -v || true | ||
|
|
||
| # Start containers | ||
| docker-compose -f docker-compose.dev.yaml up -d --build | ||
| if [ $? -ne 0 ]; then | ||
| echo "Failed to start containers" | ||
| exit 1 | ||
| fi | ||
|
|
||
| timeout=60 | ||
| until curl -f "${HEALTH_CHECK_URL:-http://localhost:4000}/health" || [ $timeout -eq 0 ]; do | ||
| echo "Waiting for API to start... ($timeout seconds remaining)" | ||
| sleep 1 | ||
| ((timeout--)) | ||
| done | ||
|
|
||
| if [ $timeout -eq 0 ]; then | ||
| echo "Error: API failed to start within timeout" | ||
| docker-compose -f docker-compose.dev.yaml logs | ||
| docker-compose -f docker-compose.dev.yaml down -v | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "API started successfully" | ||
|
|
||
| # Cleanup | ||
| docker-compose -f docker-compose.dev.yaml down -v | ||
| if [ $? -ne 0 ]; then | ||
| echo "Failed to cleanup containers" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve shell script robustness
While the implementation is good, there are some improvements needed:
- Use direct command checking instead of
$? - Fix indentation inconsistencies
- Add error handling for curl command
Apply this diff:
- name: Check if Talawa API starts in Docker
run: |
# Ensure no containers are running
docker-compose -f docker-compose.dev.yaml down -v || true
# Start containers
- docker-compose -f docker-compose.dev.yaml up -d --build
- if [ $? -ne 0 ]; then
+ if ! docker-compose -f docker-compose.dev.yaml up -d --build; then
echo "Failed to start containers"
exit 1
fi
timeout=60
- until curl -f "${HEALTH_CHECK_URL:-http://localhost:4000}/health" || [ $timeout -eq 0 ]; do
+ until curl -sf "${HEALTH_CHECK_URL:-http://localhost:4000}/health" 2>/dev/null || [ $timeout -eq 0 ]; do
echo "Waiting for API to start... ($timeout seconds remaining)"
sleep 1
((timeout--))
done
if [ $timeout -eq 0 ]; then
echo "Error: API failed to start within timeout"
docker-compose -f docker-compose.dev.yaml logs
docker-compose -f docker-compose.dev.yaml down -v
exit 1
fi
echo "API started successfully"
# Cleanup
- docker-compose -f docker-compose.dev.yaml down -v
- if [ $? -ne 0 ]; then
+ if ! docker-compose -f docker-compose.dev.yaml down -v; then
echo "Failed to cleanup containers"
exit 1
fi📝 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.
| - name: Check if Talawa API starts in Docker | |
| run: | | |
| # Ensure no containers are running | |
| docker-compose -f docker-compose.dev.yaml down -v || true | |
| # Start containers | |
| docker-compose -f docker-compose.dev.yaml up -d --build | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to start containers" | |
| exit 1 | |
| fi | |
| timeout=60 | |
| until curl -f "${HEALTH_CHECK_URL:-http://localhost:4000}/health" || [ $timeout -eq 0 ]; do | |
| echo "Waiting for API to start... ($timeout seconds remaining)" | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: API failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| echo "API started successfully" | |
| # Cleanup | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| if [ $? -ne 0 ]; then | |
| echo "Failed to cleanup containers" | |
| exit 1 | |
| fi | |
| - name: Check if Talawa API starts in Docker | |
| run: | | |
| # Ensure no containers are running | |
| docker-compose -f docker-compose.dev.yaml down -v || true | |
| # Start containers | |
| if ! docker-compose -f docker-compose.dev.yaml up -d --build; then | |
| echo "Failed to start containers" | |
| exit 1 | |
| fi | |
| timeout=60 | |
| until curl -sf "${HEALTH_CHECK_URL:-http://localhost:4000}/health" 2>/dev/null || [ $timeout -eq 0 ]; do | |
| echo "Waiting for API to start... ($timeout seconds remaining)" | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: API failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| echo "API started successfully" | |
| # Cleanup | |
| if ! docker-compose -f docker-compose.dev.yaml down -v; then | |
| echo "Failed to cleanup containers" | |
| exit 1 | |
| fi |
🧰 Tools
🪛 actionlint
304-304: shellcheck reported issue in this script: SC2181:style:6:7: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
304-304: shellcheck reported issue in this script: SC2181:style:29:7: Check exit code directly with e.g. 'if ! mycmd;', not indirectly with $?
(shellcheck)
🪛 yamllint
[error] 320-320: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
.github/workflows/pull-request.yml (1)
298-337: Remove trailing spacesThere are trailing spaces on several lines that should be removed:
- Line 298: Empty line with spaces
- Line 302: After
steps:- Line 319: After
done- Line 337: After
number }}Remove trailing spaces to improve code cleanliness.
🧰 Tools
🪛 yamllint
[error] 298-298: trailing spaces
(trailing-spaces)
[error] 302-302: trailing spaces
(trailing-spaces)
[error] 319-319: trailing spaces
(trailing-spaces)
[error] 337-337: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 yamllint
.github/workflows/pull-request.yml
[error] 298-298: trailing spaces
(trailing-spaces)
[error] 302-302: trailing spaces
(trailing-spaces)
[error] 319-319: trailing spaces
(trailing-spaces)
[error] 337-337: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
299-337: Verify Docker configuration and test data setup
Let's verify the Docker configuration and ensure test data setup is handled properly.
🧰 Tools
🪛 yamllint
[error] 302-302: trailing spaces
(trailing-spaces)
[error] 319-319: trailing spaces
(trailing-spaces)
[error] 337-337: trailing spaces
(trailing-spaces)
.github/workflows/pull-request.yml
Outdated
| Docker-Check: | ||
| needs: Test-Application | ||
| runs-on: ubuntu-latest | ||
| steps: |
There was a problem hiding this comment.
Add missing required steps
The job is missing critical setup steps:
- Repository checkout to access docker-compose files
- Node.js setup for environment consistency with other jobs
Add these steps before the Docker check:
Docker-Check:
needs: Test-Application
runs-on: ubuntu-latest
steps:
+ - name: Checkout repository
+ uses: actions/checkout@v4
+
+ - name: Set up Node.js
+ uses: actions/setup-node@v4
+ with:
+ node-version: '22.x'📝 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.
| Docker-Check: | |
| needs: Test-Application | |
| runs-on: ubuntu-latest | |
| steps: | |
| Docker-Check: | |
| needs: Test-Application | |
| runs-on: ubuntu-latest | |
| steps: | |
| - name: Checkout repository | |
| uses: actions/checkout@v4 | |
| - name: Set up Node.js | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: '22.x' |
🧰 Tools
🪛 yamllint
[error] 302-302: trailing spaces
(trailing-spaces)
.github/workflows/pull-request.yml
Outdated
| env: | ||
| HEALTH_CHECK_URL: http://localhost:4000 | ||
| COMPOSE_PROJECT_NAME: pr-${{ github.event.pull_request.number }} |
There was a problem hiding this comment.
Add missing required environment variables
The Docker environment is missing critical variables that are required for the application to start properly. These variables are present in the Test-Application job but missing here.
Add these environment variables:
env:
HEALTH_CHECK_URL: http://localhost:4000
- COMPOSE_PROJECT_NAME: pr-${{ github.event.pull_request.number }}
+ COMPOSE_PROJECT_NAME: pr-${{ github.event.pull_request.number }}
+ MONGO_DB_URL: mongodb://mongodb:27017/talawa-test-db
+ REDIS_HOST: redis
+ REDIS_PORT: 6379
+ ACCESS_TOKEN_SECRET: ${{ env.ACCESS_TOKEN_SECRET }}
+ REFRESH_TOKEN_SECRET: ${{ env.REFRESH_TOKEN_SECRET }}
+ LAST_RESORT_SUPERADMIN_EMAIL: "[email protected]"
+ COLORIZE_LOGS: "true"
+ LOG_LEVEL: "info"Note: The MongoDB and Redis hostnames are changed to match the service names in docker-compose.
📝 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.
| env: | |
| HEALTH_CHECK_URL: http://localhost:4000 | |
| COMPOSE_PROJECT_NAME: pr-${{ github.event.pull_request.number }} | |
| env: | |
| HEALTH_CHECK_URL: http://localhost:4000 | |
| COMPOSE_PROJECT_NAME: pr-${{ github.event.pull_request.number }} | |
| MONGO_DB_URL: mongodb://mongodb:27017/talawa-test-db | |
| REDIS_HOST: redis | |
| REDIS_PORT: 6379 | |
| ACCESS_TOKEN_SECRET: ${{ env.ACCESS_TOKEN_SECRET }} | |
| REFRESH_TOKEN_SECRET: ${{ env.REFRESH_TOKEN_SECRET }} | |
| LAST_RESORT_SUPERADMIN_EMAIL: "[email protected]" | |
| COLORIZE_LOGS: "true" | |
| LOG_LEVEL: "info" |
🧰 Tools
🪛 yamllint
[error] 337-337: trailing spaces
(trailing-spaces)
.github/workflows/pull-request.yml
Outdated
| - name: Check if Talawa API starts in Docker | ||
| run: | | ||
| # Ensure no containers are running | ||
| docker-compose -f docker-compose.dev.yaml down -v || true | ||
|
|
||
| # Start containers | ||
| if ! docker-compose -f docker-compose.dev.yaml up -d --build; then | ||
| echo "Failed to start containers" | ||
| exit 1 | ||
| fi | ||
|
|
||
| timeout=60 | ||
| until curl -sf "${HEALTH_CHECK_URL:-http://localhost:4000}/health" 2>/dev/null || [ $timeout -eq 0 ]; do | ||
| echo "Waiting for API to start... ($timeout seconds remaining)" | ||
| sleep 1 | ||
| ((timeout--)) | ||
| done | ||
|
|
||
| if [ $timeout -eq 0 ]; then | ||
| echo "Error: API failed to start within timeout" | ||
| docker-compose -f docker-compose.dev.yaml logs | ||
| docker-compose -f docker-compose.dev.yaml down -v | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "API started successfully" | ||
|
|
||
| # Cleanup | ||
| if ! docker-compose -f docker-compose.dev.yaml down -v; then | ||
| echo "Failed to cleanup containers" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Enhance Docker health check implementation
The current implementation has several issues:
- No verification that required services (MongoDB, Redis) are running
- Health check might fail due to container networking
- Missing proper error handling and logging
Replace the implementation with this more robust version:
- name: Check if Talawa API starts in Docker
run: |
# Ensure no containers are running
docker-compose -f docker-compose.dev.yaml down -v || true
# Start containers
if ! docker-compose -f docker-compose.dev.yaml up -d --build; then
echo "Failed to start containers"
docker-compose -f docker-compose.dev.yaml logs
exit 1
fi
# Wait for MongoDB and Redis to be ready
+ echo "Waiting for MongoDB..."
+ timeout=30
+ until docker-compose -f docker-compose.dev.yaml exec -T mongodb mongo --eval "db.runCommand('ping').ok" >/dev/null 2>&1 || [ $timeout -eq 0 ]; do
+ sleep 1
+ ((timeout--))
+ done
+
+ echo "Waiting for Redis..."
+ timeout=30
+ until docker-compose -f docker-compose.dev.yaml exec -T redis redis-cli ping >/dev/null 2>&1 || [ $timeout -eq 0 ]; do
+ sleep 1
+ ((timeout--))
+ done
# Wait for API to be healthy
timeout=60
- until curl -sf "${HEALTH_CHECK_URL:-http://localhost:4000}/health" 2>/dev/null || [ $timeout -eq 0 ]; do
+ until docker-compose -f docker-compose.dev.yaml exec -T api curl -sf "http://localhost:4000/health" 2>/dev/null || [ $timeout -eq 0 ]; do
echo "Waiting for API to start... ($timeout seconds remaining)"
sleep 1
((timeout--))
done
if [ $timeout -eq 0 ]; then
echo "Error: API failed to start within timeout"
docker-compose -f docker-compose.dev.yaml logs
docker-compose -f docker-compose.dev.yaml down -v
exit 1
fi
echo "API started successfully"
# Cleanup
if ! docker-compose -f docker-compose.dev.yaml down -v; then
echo "Failed to cleanup containers"
exit 1
fi📝 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.
| - name: Check if Talawa API starts in Docker | |
| run: | | |
| # Ensure no containers are running | |
| docker-compose -f docker-compose.dev.yaml down -v || true | |
| # Start containers | |
| if ! docker-compose -f docker-compose.dev.yaml up -d --build; then | |
| echo "Failed to start containers" | |
| exit 1 | |
| fi | |
| timeout=60 | |
| until curl -sf "${HEALTH_CHECK_URL:-http://localhost:4000}/health" 2>/dev/null || [ $timeout -eq 0 ]; do | |
| echo "Waiting for API to start... ($timeout seconds remaining)" | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: API failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| echo "API started successfully" | |
| # Cleanup | |
| if ! docker-compose -f docker-compose.dev.yaml down -v; then | |
| echo "Failed to cleanup containers" | |
| exit 1 | |
| fi | |
| - name: Check if Talawa API starts in Docker | |
| run: | | |
| # Ensure no containers are running | |
| docker-compose -f docker-compose.dev.yaml down -v || true | |
| # Start containers | |
| if ! docker-compose -f docker-compose.dev.yaml up -d --build; then | |
| echo "Failed to start containers" | |
| docker-compose -f docker-compose.dev.yaml logs | |
| exit 1 | |
| fi | |
| # Wait for MongoDB and Redis to be ready | |
| echo "Waiting for MongoDB..." | |
| timeout=30 | |
| until docker-compose -f docker-compose.dev.yaml exec -T mongodb mongo --eval "db.runCommand('ping').ok" >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| echo "Waiting for Redis..." | |
| timeout=30 | |
| until docker-compose -f docker-compose.dev.yaml exec -T redis redis-cli ping >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| # Wait for API to be healthy | |
| timeout=60 | |
| until docker-compose -f docker-compose.dev.yaml exec -T api curl -sf "http://localhost:4000/health" 2>/dev/null || [ $timeout -eq 0 ]; do | |
| echo "Waiting for API to start... ($timeout seconds remaining)" | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: API failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| echo "API started successfully" | |
| # Cleanup | |
| if ! docker-compose -f docker-compose.dev.yaml down -v; then | |
| echo "Failed to cleanup containers" | |
| exit 1 | |
| fi |
🧰 Tools
🪛 yamllint
[error] 319-319: trailing spaces
(trailing-spaces)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2653 +/- ##
========================================
Coverage 97.74% 97.74%
========================================
Files 358 358
Lines 18114 18114
Branches 2600 2599 -1
========================================
Hits 17706 17706
Misses 404 404
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
.github/workflows/pull-request.yml (1)
298-302: Fix formatting issues in job definitionRemove trailing spaces from lines 298 and 302 to maintain consistent formatting.
- + Docker-Check: needs: Test-Application runs-on: ubuntu-latest - + steps:🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 298-298: trailing spaces
(trailing-spaces)
[error] 302-302: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.3)
.github/workflows/pull-request.yml
309-309: "with" section should not be empty. please remove this section if it's unnecessary
(syntax-check)
310-310: unexpected key "node-version" for "step" section. expected one of "continue-on-error", "env", "id", "if", "name", "run", "shell", "timeout-minutes", "uses", "with", "working-directory"
(syntax-check)
366-366: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "secrets", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
367-367: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "secrets", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🪛 yamllint (1.29.0-1)
.github/workflows/pull-request.yml
[error] 298-298: trailing spaces
(trailing-spaces)
[error] 302-302: trailing spaces
(trailing-spaces)
[error] 303-303: trailing spaces
(trailing-spaces)
[error] 337-337: trailing spaces
(trailing-spaces)
[error] 344-344: trailing spaces
(trailing-spaces)
[error] 370-370: trailing spaces
(trailing-spaces)
.github/workflows/pull-request.yml
Outdated
| - name: Set up Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: '22.x' | ||
|
|
There was a problem hiding this comment.
Fix Node.js setup step syntax
The with section has incorrect indentation and syntax.
- name: Set up Node.js
uses: actions/setup-node@v4
with:
- node-version: '22.x'
+ node-version: '22.x'📝 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.
| - name: Set up Node.js | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: '22.x' | |
| - name: Set up Node.js | |
| uses: actions/setup-node@v4 | |
| with: | |
| node-version: '22.x' |
🧰 Tools
🪛 actionlint (1.7.3)
309-309: "with" section should not be empty. please remove this section if it's unnecessary
(syntax-check)
310-310: unexpected key "node-version" for "step" section. expected one of "continue-on-error", "env", "id", "if", "name", "run", "shell", "timeout-minutes", "uses", "with", "working-directory"
(syntax-check)
.github/workflows/pull-request.yml
Outdated
| env: | ||
| HEALTH_CHECK_URL: http://localhost:4000 | ||
| COMPOSE_PROJECT_NAME: pr-${{ github.event.pull_request.number }} | ||
| MONGO_DB_URL: mongodb://mongodb:27017/talawa-test-db | ||
| REDIS_HOST: redis | ||
| REDIS_PORT: 6379 | ||
| ACCESS_TOKEN_SECRET: ${{ env.ACCESS_TOKEN_SECRET }} | ||
| REFRESH_TOKEN_SECRET: ${{ env.REFRESH_TOKEN_SECRET }} | ||
| LAST_RESORT_SUPERADMIN_EMAIL: "[email protected]" | ||
| COLORIZE_LOGS: "true" | ||
| LOG_LEVEL: "info" |
There was a problem hiding this comment.
Fix environment variables configuration
- Replace
envcontext withgithubcontext for token secrets - Remove trailing spaces
env:
HEALTH_CHECK_URL: http://localhost:4000
COMPOSE_PROJECT_NAME: pr-${{ github.event.pull_request.number }}
MONGO_DB_URL: mongodb://mongodb:27017/talawa-test-db
REDIS_HOST: redis
REDIS_PORT: 6379
- ACCESS_TOKEN_SECRET: ${{ env.ACCESS_TOKEN_SECRET }}
- REFRESH_TOKEN_SECRET: ${{ env.REFRESH_TOKEN_SECRET }}
+ ACCESS_TOKEN_SECRET: ${{ github.event.repository.name }}_access_${{ github.sha }}
+ REFRESH_TOKEN_SECRET: ${{ github.event.repository.name }}_refresh_${{ github.sha }}
LAST_RESORT_SUPERADMIN_EMAIL: "[email protected]"
COLORIZE_LOGS: "true"
- LOG_LEVEL: "info"
+ LOG_LEVEL: "info"📝 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.
| env: | |
| HEALTH_CHECK_URL: http://localhost:4000 | |
| COMPOSE_PROJECT_NAME: pr-${{ github.event.pull_request.number }} | |
| MONGO_DB_URL: mongodb://mongodb:27017/talawa-test-db | |
| REDIS_HOST: redis | |
| REDIS_PORT: 6379 | |
| ACCESS_TOKEN_SECRET: ${{ env.ACCESS_TOKEN_SECRET }} | |
| REFRESH_TOKEN_SECRET: ${{ env.REFRESH_TOKEN_SECRET }} | |
| LAST_RESORT_SUPERADMIN_EMAIL: "[email protected]" | |
| COLORIZE_LOGS: "true" | |
| LOG_LEVEL: "info" | |
| env: | |
| HEALTH_CHECK_URL: http://localhost:4000 | |
| COMPOSE_PROJECT_NAME: pr-${{ github.event.pull_request.number }} | |
| MONGO_DB_URL: mongodb://mongodb:27017/talawa-test-db | |
| REDIS_HOST: redis | |
| REDIS_PORT: 6379 | |
| ACCESS_TOKEN_SECRET: ${{ github.event.repository.name }}_access_${{ github.sha }} | |
| REFRESH_TOKEN_SECRET: ${{ github.event.repository.name }}_refresh_${{ github.sha }} | |
| LAST_RESORT_SUPERADMIN_EMAIL: "[email protected]" | |
| COLORIZE_LOGS: "true" | |
| LOG_LEVEL: "info" |
🧰 Tools
🪛 actionlint (1.7.3)
366-366: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "secrets", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
367-367: context "env" is not allowed here. available contexts are "github", "inputs", "matrix", "needs", "secrets", "strategy", "vars". see https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability for more details
(expression)
🪛 yamllint (1.29.0-1)
[error] 370-370: trailing spaces
(trailing-spaces)
.github/workflows/pull-request.yml
Outdated
| - name: Check if Talawa API starts in Docker | ||
| run: | | ||
| # Ensure no containers are running | ||
| docker-compose -f docker-compose.dev.yaml down -v || true | ||
|
|
||
| # Start containers | ||
| if ! docker-compose -f docker-compose.dev.yaml up -d --build; then | ||
| echo "Failed to start containers" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Wait for MongoDB and Redis to be ready | ||
| echo "Waiting for MongoDB..." | ||
| timeout=30 | ||
| until docker-compose -f docker-compose.dev.yaml exec -T mongodb mongo --eval "db.runCommand('ping').ok" >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | ||
| sleep 1 | ||
| ((timeout--)) | ||
| done | ||
|
|
||
| echo "Waiting for Redis..." | ||
| timeout=30 | ||
| until docker-compose -f docker-compose.dev.yaml exec -T redis redis-cli ping >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | ||
| sleep 1 | ||
| ((timeout--)) | ||
| done | ||
|
|
||
| # Wait for API to be healthy | ||
| timeout=60 | ||
| until docker-compose -f docker-compose.dev.yaml exec -T api curl -sf "http://localhost:4000/health" 2>/dev/null || [ $timeout -eq 0 ]; do | ||
| echo "Waiting for API to start... ($timeout seconds remaining)" | ||
| sleep 1 | ||
| ((timeout--)) | ||
| done | ||
|
|
||
| if [ $timeout -eq 0 ]; then | ||
| echo "Error: API failed to start within timeout" | ||
| docker-compose -f docker-compose.dev.yaml logs | ||
| docker-compose -f docker-compose.dev.yaml down -v | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "API started successfully" | ||
|
|
||
| # Cleanup | ||
| if ! docker-compose -f docker-compose.dev.yaml down -v; then | ||
| echo "Failed to cleanup containers" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance Docker health check implementation
The implementation needs improvements in error handling and container networking:
- Add timeout handling for MongoDB and Redis checks
- Use container DNS names instead of localhost
# Wait for MongoDB and Redis to be ready
echo "Waiting for MongoDB..."
timeout=30
until docker-compose -f docker-compose.dev.yaml exec -T mongodb mongo --eval "db.runCommand('ping').ok" >/dev/null 2>&1 || [ $timeout -eq 0 ]; do
- sleep 1
- ((timeout--))
+ sleep 1
+ ((timeout--))
done
+ if [ $timeout -eq 0 ]; then
+ echo "Error: MongoDB failed to start within timeout"
+ docker-compose -f docker-compose.dev.yaml logs mongodb
+ docker-compose -f docker-compose.dev.yaml down -v
+ exit 1
+ fi
echo "Waiting for Redis..."
timeout=30
until docker-compose -f docker-compose.dev.yaml exec -T redis redis-cli ping >/dev/null 2>&1 || [ $timeout -eq 0 ]; do
- sleep 1
- ((timeout--))
+ sleep 1
+ ((timeout--))
done
+ if [ $timeout -eq 0 ]; then
+ echo "Error: Redis failed to start within timeout"
+ docker-compose -f docker-compose.dev.yaml logs redis
+ docker-compose -f docker-compose.dev.yaml down -v
+ exit 1
+ fi
# Wait for API to be healthy
timeout=60
- until docker-compose -f docker-compose.dev.yaml exec -T api curl -sf "http://localhost:4000/health" 2>/dev/null || [ $timeout -eq 0 ]; do
+ until docker-compose -f docker-compose.dev.yaml exec -T api curl -sf "http://api:4000/health" 2>/dev/null || [ $timeout -eq 0 ]; do📝 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.
| - name: Check if Talawa API starts in Docker | |
| run: | | |
| # Ensure no containers are running | |
| docker-compose -f docker-compose.dev.yaml down -v || true | |
| # Start containers | |
| if ! docker-compose -f docker-compose.dev.yaml up -d --build; then | |
| echo "Failed to start containers" | |
| exit 1 | |
| fi | |
| # Wait for MongoDB and Redis to be ready | |
| echo "Waiting for MongoDB..." | |
| timeout=30 | |
| until docker-compose -f docker-compose.dev.yaml exec -T mongodb mongo --eval "db.runCommand('ping').ok" >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| echo "Waiting for Redis..." | |
| timeout=30 | |
| until docker-compose -f docker-compose.dev.yaml exec -T redis redis-cli ping >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| # Wait for API to be healthy | |
| timeout=60 | |
| until docker-compose -f docker-compose.dev.yaml exec -T api curl -sf "http://localhost:4000/health" 2>/dev/null || [ $timeout -eq 0 ]; do | |
| echo "Waiting for API to start... ($timeout seconds remaining)" | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: API failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| echo "API started successfully" | |
| # Cleanup | |
| if ! docker-compose -f docker-compose.dev.yaml down -v; then | |
| echo "Failed to cleanup containers" | |
| exit 1 | |
| fi | |
| - name: Check if Talawa API starts in Docker | |
| run: | | |
| # Ensure no containers are running | |
| docker-compose -f docker-compose.dev.yaml down -v || true | |
| # Start containers | |
| if ! docker-compose -f docker-compose.dev.yaml up -d --build; then | |
| echo "Failed to start containers" | |
| exit 1 | |
| fi | |
| # Wait for MongoDB and Redis to be ready | |
| echo "Waiting for MongoDB..." | |
| timeout=30 | |
| until docker-compose -f docker-compose.dev.yaml exec -T mongodb mongo --eval "db.runCommand('ping').ok" >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: MongoDB failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs mongodb | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| echo "Waiting for Redis..." | |
| timeout=30 | |
| until docker-compose -f docker-compose.dev.yaml exec -T redis redis-cli ping >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: Redis failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs redis | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| # Wait for API to be healthy | |
| timeout=60 | |
| until docker-compose -f docker-compose.dev.yaml exec -T api curl -sf "http://api:4000/health" 2>/dev/null || [ $timeout -eq 0 ]; do | |
| echo "Waiting for API to start... ($timeout seconds remaining)" | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: API failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| echo "API started successfully" | |
| # Cleanup | |
| if ! docker-compose -f docker-compose.dev.yaml down -v; then | |
| echo "Failed to cleanup containers" | |
| exit 1 | |
| fi |
🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 337-337: trailing spaces
(trailing-spaces)
[error] 344-344: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
.github/workflows/pull-request.yml (3)
298-303: Fix formatting issues in job configurationRemove trailing spaces from the following lines:
- Line 298: Empty line with trailing space
- Line 302: After
ubuntu-latest- Line 303: Before
steps:🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 298-298: trailing spaces
(trailing-spaces)
[error] 302-302: trailing spaces
(trailing-spaces)
[error] 303-303: trailing spaces
(trailing-spaces)
304-310: Add caching for node_modulesConsider adding caching for
node_modulesto improve workflow execution time. This is especially important since the job depends onTest-Applicationwhich already has the cache.- name: Set up Node.js uses: actions/setup-node@v4 with: node-version: '22.x' + + - name: Restore node_modules from cache + id: cache-npm + uses: actions/cache@v4 + env: + cache-name: cache-node-modules + with: + path: | + ~/.npm + node_modules + key: ${{ runner.os }}-docker-check-${{ env.cache-name }}-${{ hashFiles('**/package-lock.json') }} + restore-keys: | + ${{ runner.os }}-docker-check-${{ env.cache-name }}- + ${{ runner.os }}-docker-check- + ${{ runner.os }}-
372-382: Fix environment variables configuration
- Remove trailing spaces from line 382
- Consider adding comments to document the purpose of each environment variable
- The
HEALTH_CHECK_URLis not used in the script as it uses the container DNS nameenv: - HEALTH_CHECK_URL: http://localhost:4000 COMPOSE_PROJECT_NAME: pr-${{ github.event.pull_request.number }} + # MongoDB connection string using container DNS name MONGO_DB_URL: mongodb://mongodb:27017/talawa-test-db + # Redis connection details using container DNS names REDIS_HOST: redis REDIS_PORT: 6379 + # Dynamic secrets using repository name and commit SHA ACCESS_TOKEN_SECRET: ${{ github.event.repository.name }}_access_${{ github.sha }} REFRESH_TOKEN_SECRET: ${{ github.event.repository.name }}_refresh_${{ github.sha }} + # Application configuration LAST_RESORT_SUPERADMIN_EMAIL: "[email protected]" COLORIZE_LOGS: "true" - LOG_LEVEL: "info" + LOG_LEVEL: "info"🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 382-382: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.29.0-1)
.github/workflows/pull-request.yml
[error] 298-298: trailing spaces
(trailing-spaces)
[error] 302-302: trailing spaces
(trailing-spaces)
[error] 303-303: trailing spaces
(trailing-spaces)
[error] 335-335: trailing spaces
(trailing-spaces)
[error] 349-349: trailing spaces
(trailing-spaces)
[error] 356-356: trailing spaces
(trailing-spaces)
[error] 382-382: trailing spaces
(trailing-spaces)
.github/workflows/pull-request.yml
Outdated
| - name: Check if Talawa API starts in Docker | ||
| run: | | ||
| # Ensure no containers are running | ||
| docker-compose -f docker-compose.dev.yaml down -v || true | ||
|
|
||
| # Start containers | ||
| if ! docker-compose -f docker-compose.dev.yaml up -d --build; then | ||
| echo "Failed to start containers" | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Wait for MongoDB and Redis to be ready | ||
| echo "Waiting for MongoDB..." | ||
| timeout=30 | ||
| until docker-compose -f docker-compose.dev.yaml exec -T mongodb mongo --eval "db.runCommand('ping').ok" >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | ||
| sleep 1 | ||
| ((timeout--)) | ||
| done | ||
| if [ $timeout -eq 0 ]; then | ||
| echo "Error: MongoDB failed to start within timeout" | ||
| docker-compose -f docker-compose.dev.yaml logs mongodb | ||
| docker-compose -f docker-compose.dev.yaml down -v | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "Waiting for Redis..." | ||
| timeout=30 | ||
| until docker-compose -f docker-compose.dev.yaml exec -T redis redis-cli ping >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | ||
| sleep 1 | ||
| ((timeout--)) | ||
| done | ||
| if [ $timeout -eq 0 ]; then | ||
| echo "Error: Redis failed to start within timeout" | ||
| docker-compose -f docker-compose.dev.yaml logs redis | ||
| docker-compose -f docker-compose.dev.yaml down -v | ||
| exit 1 | ||
| fi | ||
|
|
||
| # Wait for API to be healthy | ||
| timeout=60 | ||
| until docker-compose -f docker-compose.dev.yaml exec -T api curl -sf "http://api:4000/health" 2>/dev/null || [ $timeout -eq 0 ]; do | ||
| echo "Waiting for API to start... ($timeout seconds remaining)" | ||
| sleep 1 | ||
| ((timeout--)) | ||
| done | ||
|
|
||
| if [ $timeout -eq 0 ]; then | ||
| echo "Error: API failed to start within timeout" | ||
| docker-compose -f docker-compose.dev.yaml logs | ||
| docker-compose -f docker-compose.dev.yaml down -v | ||
| exit 1 | ||
| fi | ||
|
|
||
| echo "API started successfully" | ||
|
|
||
| # Cleanup | ||
| if ! docker-compose -f docker-compose.dev.yaml down -v; then | ||
| echo "Failed to cleanup containers" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix shell script implementation issues
- The shell script has inconsistent indentation which makes it harder to read
- Missing verification for docker-compose.dev.yaml
- Cleanup step could fail silently in some cases
- name: Check if Talawa API starts in Docker
run: |
# Ensure no containers are running
docker-compose -f docker-compose.dev.yaml down -v || true
+ # Verify docker-compose file exists
+ if [ ! -f "docker-compose.dev.yaml" ]; then
+ echo "Error: docker-compose.dev.yaml not found"
+ exit 1
+ fi
+
# Start containers
if ! docker-compose -f docker-compose.dev.yaml up -d --build; then
echo "Failed to start containers"
exit 1
fi
# Wait for MongoDB and Redis to be ready
echo "Waiting for MongoDB..."
timeout=30
until docker-compose -f docker-compose.dev.yaml exec -T mongodb mongo --eval "db.runCommand('ping').ok" >/dev/null 2>&1 || [ $timeout -eq 0 ]; do
sleep 1
((timeout--))
done
if [ $timeout -eq 0 ]; then
echo "Error: MongoDB failed to start within timeout"
docker-compose -f docker-compose.dev.yaml logs mongodb
docker-compose -f docker-compose.dev.yaml down -v
exit 1
fi
echo "Waiting for Redis..."
timeout=30
until docker-compose -f docker-compose.dev.yaml exec -T redis redis-cli ping >/dev/null 2>&1 || [ $timeout -eq 0 ]; do
sleep 1
((timeout--))
done
if [ $timeout -eq 0 ]; then
echo "Error: Redis failed to start within timeout"
docker-compose -f docker-compose.dev.yaml logs redis
docker-compose -f docker-compose.dev.yaml down -v
exit 1
fi
# Wait for API to be healthy
timeout=60
until docker-compose -f docker-compose.dev.yaml exec -T api curl -sf "http://api:4000/health" 2>/dev/null || [ $timeout -eq 0 ]; do
echo "Waiting for API to start... ($timeout seconds remaining)"
sleep 1
((timeout--))
done
if [ $timeout -eq 0 ]; then
echo "Error: API failed to start within timeout"
docker-compose -f docker-compose.dev.yaml logs
docker-compose -f docker-compose.dev.yaml down -v
exit 1
fi
echo "API started successfully"
# Cleanup
- if ! docker-compose -f docker-compose.dev.yaml down -v; then
- echo "Failed to cleanup containers"
- exit 1
- fi
+ # Ensure cleanup runs even if the script fails
+ cleanup() {
+ echo "Cleaning up containers..."
+ if ! docker-compose -f docker-compose.dev.yaml down -v; then
+ echo "Warning: Failed to cleanup containers"
+ fi
+ }
+ trap cleanup EXIT📝 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.
| - name: Check if Talawa API starts in Docker | |
| run: | | |
| # Ensure no containers are running | |
| docker-compose -f docker-compose.dev.yaml down -v || true | |
| # Start containers | |
| if ! docker-compose -f docker-compose.dev.yaml up -d --build; then | |
| echo "Failed to start containers" | |
| exit 1 | |
| fi | |
| # Wait for MongoDB and Redis to be ready | |
| echo "Waiting for MongoDB..." | |
| timeout=30 | |
| until docker-compose -f docker-compose.dev.yaml exec -T mongodb mongo --eval "db.runCommand('ping').ok" >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: MongoDB failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs mongodb | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| echo "Waiting for Redis..." | |
| timeout=30 | |
| until docker-compose -f docker-compose.dev.yaml exec -T redis redis-cli ping >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: Redis failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs redis | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| # Wait for API to be healthy | |
| timeout=60 | |
| until docker-compose -f docker-compose.dev.yaml exec -T api curl -sf "http://api:4000/health" 2>/dev/null || [ $timeout -eq 0 ]; do | |
| echo "Waiting for API to start... ($timeout seconds remaining)" | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: API failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| echo "API started successfully" | |
| # Cleanup | |
| if ! docker-compose -f docker-compose.dev.yaml down -v; then | |
| echo "Failed to cleanup containers" | |
| exit 1 | |
| fi | |
| - name: Check if Talawa API starts in Docker | |
| run: | | |
| # Ensure no containers are running | |
| docker-compose -f docker-compose.dev.yaml down -v || true | |
| # Verify docker-compose file exists | |
| if [ ! -f "docker-compose.dev.yaml" ]; then | |
| echo "Error: docker-compose.dev.yaml not found" | |
| exit 1 | |
| fi | |
| # Start containers | |
| if ! docker-compose -f docker-compose.dev.yaml up -d --build; then | |
| echo "Failed to start containers" | |
| exit 1 | |
| fi | |
| # Wait for MongoDB and Redis to be ready | |
| echo "Waiting for MongoDB..." | |
| timeout=30 | |
| until docker-compose -f docker-compose.dev.yaml exec -T mongodb mongo --eval "db.runCommand('ping').ok" >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: MongoDB failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs mongodb | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| echo "Waiting for Redis..." | |
| timeout=30 | |
| until docker-compose -f docker-compose.dev.yaml exec -T redis redis-cli ping >/dev/null 2>&1 || [ $timeout -eq 0 ]; do | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: Redis failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs redis | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| # Wait for API to be healthy | |
| timeout=60 | |
| until docker-compose -f docker-compose.dev.yaml exec -T api curl -sf "http://api:4000/health" 2>/dev/null || [ $timeout -eq 0 ]; do | |
| echo "Waiting for API to start... ($timeout seconds remaining)" | |
| sleep 1 | |
| ((timeout--)) | |
| done | |
| if [ $timeout -eq 0 ]; then | |
| echo "Error: API failed to start within timeout" | |
| docker-compose -f docker-compose.dev.yaml logs | |
| docker-compose -f docker-compose.dev.yaml down -v | |
| exit 1 | |
| fi | |
| echo "API started successfully" | |
| # Cleanup | |
| # Ensure cleanup runs even if the script fails | |
| cleanup() { | |
| echo "Cleaning up containers..." | |
| if ! docker-compose -f docker-compose.dev.yaml down -v; then | |
| echo "Warning: Failed to cleanup containers" | |
| fi | |
| } | |
| trap cleanup EXIT |
🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 335-335: trailing spaces
(trailing-spaces)
[error] 349-349: trailing spaces
(trailing-spaces)
[error] 356-356: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.github/workflows/pull-request.yml (3)
298-311: Fix formatting and optimize Node.js setup
- Remove trailing spaces on lines 298, 302, and 303
- Consider caching Node.js dependencies for faster builds
Add the cache step after Node.js setup:
- name: Set up Node.js uses: actions/setup-node@v4 with: node-version: '22.x' + + - name: Cache Node.js dependencies + uses: actions/cache@v4 + with: + path: | + ~/.npm + node_modules + key: ${{ runner.os }}-docker-check-${{ hashFiles('**/package-lock.json') }} + restore-keys: | + ${{ runner.os }}-docker-check-🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 298-298: trailing spaces
(trailing-spaces)
[error] 302-302: trailing spaces
(trailing-spaces)
[error] 303-303: trailing spaces
(trailing-spaces)
375-385: Add missing environment variables and fix formatting
- Remove trailing spaces on line 385
- Add missing environment variables for complete configuration
Add these environment variables:
env: HEALTH_CHECK_URL: http://localhost:4000 COMPOSE_PROJECT_NAME: pr-${{ github.event.pull_request.number }} MONGO_DB_URL: mongodb://mongodb:27017/talawa-test-db REDIS_HOST: redis REDIS_PORT: 6379 ACCESS_TOKEN_SECRET: ${{ github.event.repository.name }}_access_${{ github.sha }} REFRESH_TOKEN_SECRET: ${{ github.event.repository.name }}_refresh_${{ github.sha }} LAST_RESORT_SUPERADMIN_EMAIL: "[email protected]" COLORIZE_LOGS: "true" - LOG_LEVEL: "info" + LOG_LEVEL: "info" + RECAPTCHA_SITE_KEY: ${{secrets.RECAPTCHA_SITE_KEY}} + RECAPTCHA_SECRET_KEY: ${{secrets.RECAPTCHA_SECRET_KEY}} + MAIL_USERNAME: ${{secrets.MAIL_USERNAME}} + MAIL_PASSWORD: ${{secrets.MAIL_PASSWORD}}🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 385-385: trailing spaces
(trailing-spaces)
299-386: Overall implementation meets requirementsThe Docker-Check job successfully implements the PR objective of verifying that the Talawa API starts correctly in Docker. The implementation includes:
- Proper dependency on Test-Application job
- Robust health checks for all services
- Proper cleanup handling
- Appropriate environment configuration
Please address the suggested improvements in the previous comments for better reliability and maintainability.
Consider adding these enhancements in future PRs:
- Parallel execution of Test-Application and Docker-Check jobs
- Reusable workflow for Docker checks
- Artifact sharing between jobs for test results
🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 302-302: trailing spaces
(trailing-spaces)
[error] 303-303: trailing spaces
(trailing-spaces)
[error] 335-335: trailing spaces
(trailing-spaces)
[error] 349-349: trailing spaces
(trailing-spaces)
[error] 356-356: trailing spaces
(trailing-spaces)
[error] 385-385: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.github/workflows/pull-request.yml(1 hunks)
🧰 Additional context used
🪛 yamllint (1.29.0-1)
.github/workflows/pull-request.yml
[error] 298-298: trailing spaces
(trailing-spaces)
[error] 302-302: trailing spaces
(trailing-spaces)
[error] 303-303: trailing spaces
(trailing-spaces)
[error] 335-335: trailing spaces
(trailing-spaces)
[error] 349-349: trailing spaces
(trailing-spaces)
[error] 356-356: trailing spaces
(trailing-spaces)
[error] 385-385: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
.github/workflows/pull-request.yml (1)
312-374: LGTM with existing suggestions
The implementation is fundamentally sound. Please refer to the existing review comments for improvements in:
- Shell script robustness
- Error handling
- Container networking
🧰 Tools
🪛 yamllint (1.29.0-1)
[error] 335-335: trailing spaces
(trailing-spaces)
[error] 349-349: trailing spaces
(trailing-spaces)
[error] 356-356: trailing spaces
(trailing-spaces)
What kind of change does this PR introduce?
refactoring
2638
Fixes #2638
If relevant, did you update the documentation?
Summary
This pr ensures that the pr workflow checks whether the talawa api app starts in docker and the workflow passes
Have you read the contributing guide?
Summary by CodeRabbit
New Features
Docker-Check, to improve Docker container management during workflows.Improvements