Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fixed pr workflow which ensures that talawa Api app starts in docker #2653

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

fixed pr workflow which ensures that talawa Api app starts in docker #2653

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7e91ca2
fixed pr workflow which ensures that talawa api app starts in docker
rahulrana701 Nov 12, 2024
0fd01f2
requested changes made
rahulrana701 Nov 12, 2024
8952134
changes made
rahulrana701 Nov 12, 2024
2288116
done as suggested
rahulrana701 Nov 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .github/workflows/pull-request.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,14 @@ jobs:

- name: Run the tests
run: npm run test

- 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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:

  1. Split into separate jobs:
    • Test-Application: Run tests locally (existing)
    • Docker-Check: Run Docker validation (new)
  2. 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Critical issues in Docker startup check implementation

The current implementation has several issues that need to be addressed:

  1. The Docker Compose command path is incorrect and non-standard
  2. Missing detached mode will cause the workflow to hang
  3. No health check to verify successful startup
  4. No timeout handling
  5. No cleanup of Docker resources
  6. 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 -v

This implementation:

  • Uses standard docker-compose command
  • 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.

Suggested change
- 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


- name: Start the development server
run: |
Expand Down