Conversation
…nd Hello Service (cherry picked from commit 5c3f15e)
- Add named volume for sharing descriptor files between services in docker-compose.yml - Update Dockerfile to generate and copy FileDescriptorSet for ggRMCP Gateway - Modify health check endpoint to support HEAD requests (cherry picked from commit ce1d073)
(cherry picked from commit 2c05c40)
There was a problem hiding this comment.
Pull request overview
This pull request adds Docker and Docker Compose support to the ggRMCP project with optional China mirror configuration to improve build speeds and accessibility for users in China. The implementation includes multi-stage builds for both the gateway and example service, with conditional mirror selection via the USE_CHINA_MIRROR build argument.
Changes:
- Added Dockerfiles for the main ggRMCP gateway and hello-service example with multi-stage builds
- Added docker-compose.yml for orchestrating the gateway and example service together
- Added .dockerignore files to optimize build context
- Updated health check endpoint to support HEAD method in addition to GET
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| Dockerfile | Multi-stage Docker build for ggRMCP gateway with optional China mirrors |
| .dockerignore | Build context exclusions for the main gateway |
| examples/hello-service/Dockerfile | Multi-stage build for example gRPC service with protobuf generation |
| examples/hello-service/.dockerignore | Build context exclusions for the example service |
| docker-compose.yml | Orchestration configuration for gateway and example service |
| cmd/grmcp/main.go | Added HEAD method support to health check endpoint |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| # Install ONLY runtime dependencies (ca-certificates for HTTPS, tzdata for timezone, wget for healthcheck) | ||
| # NO git, gcc, make, or musl-dev! | ||
| RUN apk add --no-cache ca-certificates tzdata wget curl |
There was a problem hiding this comment.
Both curl and wget are installed in the runner stage, but only wget is used in the healthcheck. Installing both adds unnecessary bloat to the final image (approximately 1-2 MB). Consider removing curl unless it's needed for other purposes.
| RUN apk add --no-cache ca-certificates tzdata wget curl | |
| RUN apk add --no-cache ca-certificates tzdata wget |
|
|
||
| // Health check endpoint | ||
| router.HandleFunc("/health", handler.HealthHandler).Methods("GET") | ||
| router.HandleFunc("/health", handler.HealthHandler).Methods("GET", "HEAD") |
There was a problem hiding this comment.
Adding HEAD method support to the health check endpoint is a good practice for Docker health checks and monitoring tools. However, the HealthHandler implementation performs full health checks (gRPC connection check and service discovery) which will execute for HEAD requests even though the response body is discarded. Consider optimizing by detecting HEAD requests and skipping the response body encoding, or by providing a lighter-weight health check for HEAD requests.
| router.HandleFunc("/health", handler.HealthHandler).Methods("GET", "HEAD") | |
| router.HandleFunc("/health", handler.HealthHandler).Methods("GET") | |
| router.HandleFunc("/health", func(w http.ResponseWriter, r *http.Request) { | |
| // Lightweight handler for HEAD requests: avoid full health check and body encoding | |
| w.WriteHeader(http.StatusOK) | |
| }).Methods("HEAD") |
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | ||
| CMD nc -z localhost 50051 || exit 1 |
There was a problem hiding this comment.
The nc (netcat) command is not available by default in Alpine Linux images. The HEALTHCHECK will fail because nc is not installed. You need to either install netcat with 'apk add --no-cache netcat-openbsd' in the RUN command at line 82, or use an alternative health check approach.
| args: | ||
| VERSION: ${VERSION:-latest} |
There was a problem hiding this comment.
The VERSION build argument is referenced in the Dockerfile but is not being properly passed through the build process. The ldflags at line 45 of the Dockerfile use ${VERSION:-dev}, which will default to 'dev'. While the docker-compose.yml sets VERSION as an environment variable and passes it as a build arg, this approach relies on the VERSION environment variable being set in the host environment. Consider explicitly setting a default value in the docker-compose.yml args section (e.g., 'VERSION: latest') or documenting that users need to export VERSION before running docker-compose.
| EXPOSE 50053 | ||
|
|
||
| # Health check | ||
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | ||
| CMD wget --no-verbose --tries=1 --spider http://localhost:50053/health || exit 1 | ||
|
|
||
| # Environment variables with defaults | ||
| ENV GRPC_HOST=localhost \ | ||
| GRPC_PORT=50051 \ | ||
| HTTP_PORT=50053 \ |
There was a problem hiding this comment.
The default HTTP_PORT in cmd/grmcp/main.go is 50052, but the Dockerfile sets ENV HTTP_PORT=50053 and docker-compose.yml uses 50053. This creates an inconsistency. While the environment variable will override the flag default when the container runs, it's better to align these values for clarity and to prevent confusion when the application is run outside Docker.
| EXPOSE 50053 | |
| # Health check | |
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | |
| CMD wget --no-verbose --tries=1 --spider http://localhost:50053/health || exit 1 | |
| # Environment variables with defaults | |
| ENV GRPC_HOST=localhost \ | |
| GRPC_PORT=50051 \ | |
| HTTP_PORT=50053 \ | |
| EXPOSE 50052 | |
| # Health check | |
| HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \ | |
| CMD wget --no-verbose --tries=1 --spider http://localhost:50052/health || exit 1 | |
| # Environment variables with defaults | |
| ENV GRPC_HOST=localhost \ | |
| GRPC_PORT=50051 \ | |
| HTTP_PORT=50052 \ |
| # For others: default proxy.golang.org | ||
| ENV GO111MODULE=on | ||
| RUN if [ "$USE_CHINA_MIRROR" = "true" ]; then \ | ||
| go env -w GOPROXY=https://goproxy.cn,https://goproxy.io,direct; \ |
There was a problem hiding this comment.
The goproxy.io mirror in the GOPROXY setting is a third-party service. While goproxy.cn is well-known and maintained by Qiniu Cloud, goproxy.io's reliability and security posture may vary. Consider using only goproxy.cn (which is sufficient for China users) or adding a comment about the security implications of using third-party proxy services.
| go env -w GOPROXY=https://goproxy.cn,https://goproxy.io,direct; \ | |
| go env -w GOPROXY=https://goproxy.cn,direct; \ |
| networks: | ||
| - grpc-net | ||
| healthcheck: | ||
| test: ["CMD", "nc", "-z", "localhost", "50051"] |
There was a problem hiding this comment.
The nc (netcat) command is not available by default in Alpine Linux. The healthcheck for hello-service will fail. You need to install netcat-openbsd in the hello-service Dockerfile or change the healthcheck command to use an available tool.
| test: ["CMD", "nc", "-z", "localhost", "50051"] | |
| test: ["CMD", "wget", "--no-verbose", "--tries=1", "--spider", "http://localhost:50051"] |
| # Use shell form to allow environment variable expansion | ||
| # The ${DESCRIPTOR_PATH:+--descriptor=${DESCRIPTOR_PATH}} syntax adds the flag only if DESCRIPTOR_PATH is set | ||
| CMD /app/grmcp \ | ||
| --grpc-host=${GRPC_HOST} \ | ||
| --grpc-port=${GRPC_PORT} \ | ||
| --http-port=${HTTP_PORT} \ | ||
| --log-level=${LOG_LEVEL} \ | ||
| ${DESCRIPTOR_PATH:+--descriptor=${DESCRIPTOR_PATH}} |
There was a problem hiding this comment.
The shell parameter expansion syntax ${DESCRIPTOR_PATH:+--descriptor=${DESCRIPTOR_PATH}} will not work in CMD exec form. This syntax requires a shell to interpret it. Either change to shell form (which you already have with the /app/grmcp command) or use an entrypoint script. Since you're already using shell form by not wrapping the command in JSON array brackets, the current implementation should work, but the comment at line 98 is misleading - this is already shell form, not a change to shell form.
| # ============================================ | ||
| # Runner Stage - Minimal runtime environment | ||
| # ============================================ | ||
| FROM alpine:latest AS runner |
There was a problem hiding this comment.
Alpine 3.19 is specified for the runner stage, but 'alpine:latest' is used in the hello-service Dockerfile. For consistency and reproducibility, it's recommended to use a specific version tag (e.g., alpine:3.19) in both Dockerfiles. Using 'latest' can lead to unpredictable behavior when the latest version changes.
| FROM alpine:latest AS runner | |
| FROM alpine:3.19 AS runner |
|
|
||
| # Documentation (not needed in runtime) | ||
| *.md | ||
| !README.md |
There was a problem hiding this comment.
The .dockerignore file excludes README.md with '!README.md' (line 34) but then also excludes all .md files with '.md' (line 33). Since '.md' comes first, the negation pattern '!README.md' will have no effect. If you want to include README.md, you should reverse the order: put '!README.md' after '*.md'. However, since the comment indicates documentation is not needed in runtime, this appears to be unintentional.
| !README.md |
👋 Introduction
Hi! I'm really excited about ggRMCP - it's an elegant solution for bridging gRPC with AI through MCP. I'd love to contribute to making it more accessible globally.
🎯 Changes
Added optional China mirror support for Docker builds via
USE_CHINA_MIRRORbuild argument:false💡 Why This Matters
Users in China face severe connectivity issues (timeouts, slow speeds, build failures). This change:
🚀 Future Contribution Plans
I'm passionate about this project and plan to contribute: