-
Notifications
You must be signed in to change notification settings - Fork 232
feat: Add Docker support and configure for Cloud Run #57
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
This change adds the necessary files and configuration to allow the application to be run in a Docker container and deployed to Google Cloud Run. - Adds a Dockerfile to containerize the application. - Adds a .dockerignore file to exclude unnecessary files from the Docker image. - Updates the server to listen on the host and port specified by the HOST and PORT environment variables, which is a requirement for Cloud Run. - Sets the transport to "streamable-http" for production environments. - Updates the README.md with instructions on how to build and run the application using Docker.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
jradcliff
left a comment
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.
Thanks for the PR! Left a few comments and questions for you.
Cheers,
Josh, Google Analytics team
| # the PORT environment variable. | ||
| host = os.environ.get("HOST", "127.0.0.1") | ||
| port = int(os.environ.get("PORT", "8000")) | ||
| mcp.run(transport="streamable-http", host=host, port=port) |
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.
Does this require an upgrade to the mcp[cli] dependency? I don't see the streamable-http option in v1.2.0:
| # the PORT environment variable. | ||
| host = os.environ.get("HOST", "127.0.0.1") | ||
| port = int(os.environ.get("PORT", "8000")) | ||
| mcp.run(transport="streamable-http", host=host, port=port) |
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.
host and port don't appear to be valid keyword args for the run() method, even in the main branch:
Should the host and port override go in the constructor call instead?
| host = os.environ.get("HOST", "127.0.0.1") | ||
| port = int(os.environ.get("PORT", "8000")) |
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.
Instead of hardcoding the defaults used by mcp here (127,0.0.1 and 8000), could you make overriding the host and port only happen if the environment has the HOST and PORT env vars set?
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.
It turns out that FastMCP already handles host/port overrides via its own env vars FAST_MCP_HOST and FAST_MCP_PORT, respectively. See this comment in server.py.
If you use those in Docker, you won't need this code that overrides the host and port at all.
| host = os.environ.get("HOST", "127.0.0.1") | ||
| port = int(os.environ.get("PORT", "8000")) |
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.
Also, HOST and PORT are pretty general names that might conflict with existing env vars. WDYT of naming these MCP_HOST and MCP_PORT instead? Then users not using Docker could also override these values just for the MCP server, without conflicts.
|
@tripwire24 Any chance you'll have time to answer the questions Jeff had / update this PR to be merged? |
This change adds the necessary files and configuration to allow the application to be run in a Docker container and deployed to Google Cloud Run.