-
Notifications
You must be signed in to change notification settings - Fork 722
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
Add Docker support for sample mcp servers #368
Conversation
2e55e9d
to
de059f1
Compare
* add Dockerfiles and update README.md definitions
* interpolation of env into command args is not supported
- Remove `$` interpolation for env - Allow puppeteer to work headless in docker, headful with npx
2c14f4a
to
8084b2b
Compare
8084b2b
to
70e19c4
Compare
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.
Thank you for working on this. Quite excited.
#### Docker | ||
|
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.
We likely want ot highlight the difference between the npx and the docker version (mostly just passing credentials (which i think in the explizit form is nicer).
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.
Ya, we've added a section on the explicit authentication step (which is also a docker cli command). This is a prerequisite step before adding the gdrive server to mcpServers
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 section is right below this thread:
https://github.com/modelcontextprotocol/servers/pull/368/files#diff-4388c32ec9267085688e3653add6606627eecdac28c0748c171040499695493eR55
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.
Great thank you.
Co-authored-by: David Soria Parra <[email protected]>
src/gdrive/README.md
Outdated
@@ -31,8 +31,9 @@ The server provides access to Google Drive files: | |||
3. [Configure an OAuth consent screen](https://console.cloud.google.com/apis/credentials/consent) ("internal" is fine for testing) | |||
4. Add OAuth scope `https://www.googleapis.com/auth/drive.readonly` | |||
5. [Create an OAuth Client ID](https://console.cloud.google.com/apis/credentials/oauthclient) for application type "Desktop App" | |||
6. Download the JSON file of your client's OAuth keys | |||
7. Rename the key file to `gcp-oauth.keys.json` and place into the root of this repo (i.e. `servers/gcp-oauth.keys.json`) | |||
6. Add http://localhost:3000/oauth2callback as a redirect URI |
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.
@dsp-ant Hope you don't mind, but I found this extra step was important to get the mcp server to work regardless of Docker/npx
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.
Thank you for noticing. I think is fine.
|
||
# Basic script to replace opn(authorizeUrl, { wait: false }).then(cp => cp.unref()); with process.stdout.write(`Open this URL in your browser: ${authorizeUrl}`); | ||
|
||
sed -i 's/opn(authorizeUrl, { wait: false }).then(cp => cp.unref());/process.stderr.write(`Open this URL in your browser: ${authorizeUrl}\n`);/' node_modules/@google-cloud/local-auth/build/src/index.js |
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 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.
Thank you so much for getting this over the finish line. I am super stoked to have this and can't wait for people to work with MCP servers that can take really interesting dependencies because of docker containers.
A few suggestions:
- We should probably add a note about the usage of Docker to the top-level readme, including a link to the mcp namespace on docker hub.
- I would love to see a section in docs/ on how to build MCP servers with docker.
- I'd love if we would say on the README that this is maintained by Docker itself.
A few questions:
- How should we deal with CI? Do we need to add anything to our .github/workflows to trigger builds?
- Where would users report errors about docker setup. We can help with maintenance but I'd love if there you all could help out. Happy to redirect them to a docker repo or if you want a docker tag in this repo to follow.
src/gdrive/README.md
Outdated
@@ -31,8 +31,9 @@ The server provides access to Google Drive files: | |||
3. [Configure an OAuth consent screen](https://console.cloud.google.com/apis/credentials/consent) ("internal" is fine for testing) | |||
4. Add OAuth scope `https://www.googleapis.com/auth/drive.readonly` | |||
5. [Create an OAuth Client ID](https://console.cloud.google.com/apis/credentials/oauthclient) for application type "Desktop App" | |||
6. Download the JSON file of your client's OAuth keys | |||
7. Rename the key file to `gcp-oauth.keys.json` and place into the root of this repo (i.e. `servers/gcp-oauth.keys.json`) | |||
6. Add http://localhost:3000/oauth2callback as a redirect URI |
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.
Thank you for noticing. I think is fine.
#### Docker | ||
|
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.
Great thank you.
@@ -0,0 +1,5 @@ | |||
#! /bin/bash | |||
|
|||
# Basic script to replace opn(authorizeUrl, { wait: false }).then(cp => cp.unref()); with process.stdout.write(`Open this URL in your browser: ${authorizeUrl}`); |
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.
Maybe in a follow up, let's add the reasoning you described. future me will thank if this is inside the bash script instead of a github comment :)
Description
Server Details
Motivation and Context
This is an additional option for running mcp servers that addresses a few pain points:
How Has This Been Tested?
We are testing with a claude_desktop_config.json containing all 18 servers. We are also adding additional entries to each of the server README.md projects to describe how to make the optional docker-based client configuration. The list of tests we have to describe here is quite long since we have to re-test all 18 servers.
Breaking Changes
No, this will not impact any existing installs. It is an orthogonal, and new, way to install and run the servers.
Types of changes
Checklist
Additional context