Add GitHub Actions workflow and dockerignore #8
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds CI/CD infrastructure for Docker-based deployments by introducing a GitHub Actions workflow and a .dockerignore file to optimize the Docker build context. While the PR description mentions adding gunicorn as a dependency, the actual changes focus on the build and test automation setup.
- GitHub Actions workflow with Linux and macOS build jobs
- Comprehensive .dockerignore to exclude unnecessary files from Docker builds
- Basic integration testing for the Docker image on Linux
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| .github/workflows/build.yml | Adds CI workflow for building and testing Docker images on both Ubuntu and macOS runners, with platform-specific Docker setup |
| .dockerignore | Excludes development files, build artifacts, and system files from the Docker build context to optimize image builds |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/build.yml
Outdated
| - name: Validate Dockerfile syntax | ||
| run: | | ||
| # Install hadolint for Dockerfile linting | ||
| brew install hadolint |
There was a problem hiding this comment.
The Colima configuration allocates 2 CPUs and 4GB of memory, but there's no error handling if Colima fails to start. Consider adding error checking or a verification step after 'colima start' to ensure the Docker environment is ready before attempting to build.
|
|
||
| on: | ||
| push: | ||
| branches: [ main, feat/* ] |
There was a problem hiding this comment.
The workflow triggers on branches matching 'feat/', but GitHub Actions branch patterns use 'feat/**' for wildcard matching of all branches under feat/. The single asterisk pattern 'feat/' will only match branches like 'feat/x' but not 'feat/x/y'.
| branches: [ main, feat/* ] | |
| branches: [ main, feat/** ] |
| -t memu-server . | ||
|
|
||
| - name: Test image | ||
| if: github.event_name == 'push' |
There was a problem hiding this comment.
The 'Test image' step only runs on push events, but it would be valuable to test the image on pull_request events as well to catch issues before merging. Consider removing or adjusting this condition to ensure image functionality is validated in PRs.
| sleep 5 | ||
| curl -f http://localhost:8005 || exit 1 |
There was a problem hiding this comment.
The test step exposes port 8005 on the host but the container runs on port 8000, then curls localhost:8005. However, there's no health check or retry logic. The 5-second sleep may not be sufficient for the application to fully start up, especially if it needs to initialize the MemoryService or download models. Consider adding a retry loop with a timeout or using a proper health check endpoint.
| sleep 5 | |
| curl -f http://localhost:8005 || exit 1 | |
| timeout=60 | |
| start_time=$(date +%s) | |
| until curl -fsS http://localhost:8005 >/dev/null 2>&1; do | |
| current_time=$(date +%s) | |
| elapsed=$((current_time - start_time)) | |
| if [ "$elapsed" -ge "$timeout" ]; then | |
| echo "Service did not become ready within ${timeout}s" | |
| docker logs test || true | |
| docker stop test || true | |
| exit 1 | |
| fi | |
| echo "Waiting for service to be ready... (${elapsed}s elapsed)" | |
| sleep 5 | |
| done | |
| curl -f http://localhost:8005 |
.github/workflows/build.yml
Outdated
|
|
||
| - name: Check Dockerfile exists and is valid | ||
| run: | | ||
| if [ -f dockerfiles/Dockerfile ]; then | ||
| echo "✅ Dockerfile found" |
There was a problem hiding this comment.
The Mac build job lacks a test step similar to the Linux job. For consistency and comprehensive testing across platforms, consider adding a test step here as well to verify the Docker image works correctly on macOS.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Test image | ||
| if: github.event_name == 'push' | ||
| run: | | ||
| docker run -d -p 8005:8000 \ | ||
| -e OPENAI_API_KEY="${{ secrets.OPENAI_API_KEY }}" \ | ||
| --name test memu-server | ||
| sleep 5 | ||
| curl -f http://localhost:8005 || exit 1 | ||
| docker logs test | ||
| docker stop test |
There was a problem hiding this comment.
The test step only runs on push events (line 25), which means pull requests won't be tested. This creates a gap in CI coverage where pull request changes won't be validated before merging. Consider removing or adjusting this condition to ensure tests run for both push and pull_request events.
| build-linux-arm: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up QEMU for multi-architecture | ||
| uses: docker/setup-qemu-action@v3 | ||
|
|
||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 | ||
|
|
||
| - name: Build Docker image for ARM64 | ||
| run: | | ||
| docker buildx build --platform linux/arm64 \ | ||
| -f dockerfiles/Dockerfile \ | ||
| -t memu-server:arm64 \ | ||
| --load \ | ||
| . No newline at end of file |
There was a problem hiding this comment.
The ARM64 build job does not include any test step to verify the built image, unlike the AMD64 build job. Consider adding a similar test step for the ARM64 image to ensure cross-platform compatibility and catch architecture-specific issues.
| -t memu-server . | ||
|
|
||
| - name: Test image | ||
| if: github.event_name == 'push' |
There was a problem hiding this comment.
The workflow expects the OPENAI_API_KEY secret to be available but doesn't handle the case where it might be missing. Consider adding a conditional check or using a continue-on-error flag to prevent the workflow from failing when the secret is not configured in forks or other repositories.
| if: github.event_name == 'push' | |
| if: github.event_name == 'push' && secrets.OPENAI_API_KEY != '' |
Add gunicorn dependency for Docker deployment