-
Notifications
You must be signed in to change notification settings - Fork 31
[DO NOT SUBMIT] Add dockerfile #133
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.
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @gmechali, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a Dockerfile to containerize the application, making it easily deployable and manageable in containerized environments. The Dockerfile sets up a Python 3.11 slim environment, installs the necessary package, configures logging and port exposure, and includes a health check to ensure the application's readiness. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a Dockerfile to containerize the application. While this is a great addition, the current implementation has several areas for improvement regarding security, correctness, and Docker best practices. My review provides suggestions to build the image from local source code instead of PyPI, fix a critical issue with the HEALTHCHECK command, run the container as a non-root user for better security, and ensure the application port is configurable via environment variables as is standard for containerized applications.
|
|
||
| # HEALTHCHECK: Basic check to ensure the server is responsive | ||
| # This helps Cloud Run know if the container is zombie | ||
| HEALTHCHECK CMD curl --fail http://localhost:8080/health || exit 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The python:3.11-slim base image does not include curl, which will cause the HEALTHCHECK to fail and may lead to the container being constantly restarted by an orchestrator like Cloud Run. You need to install curl first. Additionally, the port is hardcoded, and it's better to use the $PORT environment variable. I've also added some recommended flags to the HEALTHCHECK for better reliability.
# Install curl for healthcheck
RUN apt-get update && apt-get install -y curl --no-install-recommends && rm -rf /var/lib/apt/lists/*
# HEALTHCHECK: Basic check to ensure the server is responsive
# This helps Cloud Run know if the container is zombie
HEALTHCHECK --interval=30s --timeout=3s --start-period=5s --retries=3 \
CMD curl -f http://localhost:${PORT}/health || exit 1
| ENV PYTHONUNBUFFERED=1 | ||
|
|
||
| # Install the package | ||
| RUN pip install --no-cache-dir datacommons-mcp |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Dockerfile currently installs datacommons-mcp from PyPI. This means any local changes made in this repository won't be included in the Docker image, making it difficult to test new code. To build an image with the current source code, you should copy the package source into the image and install it from there.
WORKDIR /app
# Copy the package source code
COPY ./packages/datacommons-mcp /app
# Install the package from the copied source
RUN pip install --no-cache-dir .
| # THE COMMAND | ||
| # 1. Bind to 0.0.0.0 (Crucial for Cloud Run) | ||
| # 2. Bind to the environment variable PORT | ||
| CMD ["datacommons-mcp", "serve", "http", "--host", "0.0.0.0", "--port", "8080"] No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two issues here:
- Security: Running containers as the
rootuser is a security risk. It's a best practice to create and switch to a non-root user. - Configuration: The port
8080is hardcoded. This prevents overriding the port using thePORTenvironment variable, which is a standard practice for containerized applications. The exec form ofCMDdoes not perform variable substitution, so you need to usesh -cto use the$PORTvariable.
My suggestion addresses both points.
# Create and switch to a non-root user
RUN addgroup --system app && adduser --system --ingroup app app
USER app
# THE COMMAND
# 1. Bind to 0.0.0.0 (Crucial for Cloud Run)
# 2. Bind to the environment variable PORT
CMD ["sh", "-c", "datacommons-mcp serve http --host 0.0.0.0 --port $PORT"]
No description provided.