-
Notifications
You must be signed in to change notification settings - Fork 564
Containerising inspector #257
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
Conversation
We really need something in the "how has this been tested" section of these PRs, particularly if it hooks into CI. |
I'll update that section, but it's mainly running a docker build and starting a container. Testing the CI pipeline is a bit harder, since it's artifact publishing, so I'm just using the instructions from the GitHub docs. |
Got it. But I don't want to break the inspector with an untested workflow. Can you please test it in your fork and take screenshots of every step? |
* Initial work for adding a Dockerfile and creating an image for inspector Fixes modelcontextprotocol#237 * Updating workflow to publish to GitHub Container Registry Using guide from https://docs.github.com/en/actions/use-cases-and-examples/publishing-packages/publishing-docker-images
@cliffhall I've updated the issue description with how I've tests it and links to the Actions run on my fork that I used. Here's some screenshot of the image on my fork: And the workflow run (sorry, it's pretty long, so I'll break it a bit 🤣): |
@aaronpowell thanks for doing that testing, I really appreciate it. Will give it closer eyes soon. |
Let me know if you need any more details on the workflow or the life cycle. |
Dropping a note here too - looking into how we might get this pushed as an official image on Docker hub. (There appears to be an old version published there that is about ~4 months old.) |
@aaronpowell This is what I get when I try to pull your container image with: docker pull ghcr.io/aaronpowell/inspector:sha256-8fe64aece8867d5c619e6910f1aa7f7c3c4267bb7374a42ec44bc6ab028f4ea5 ![]() ![]() |
I got the same error as above when pulling the container with the docker pull ghcr.io/aaronpowell/inspector |
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.
@aaronpowell this is looking good. There is some setup outside the files in this repo though. Will look into it.
Dockerfile
Outdated
@@ -0,0 +1,26 @@ | |||
FROM node:22-slim |
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's likely advantageous to have separate build and runtime stages:
# layer 1 - build stage
FROM node:22-slim AS build
# copies all the files over and installs dependencies
# actually compile the typescript which emits the "server/build/" and "client/dist/"
RUN npm run build
# layer 2 - runtime stage with the compiled /bundled javascript
FROM node:22-slim
WORKDIR /app
# copy over all build / dist files (including node_module dependencies)
COPY --from=build /app/client/dist ./client/dist
COPY --from=build /app/server/build ./server/build
COPY --from=build /app/node_modules ./node_modules
# etc. etc. rest of dockerfile
Dockerfile
Outdated
# Install dependencies | ||
# Working around https://github.com/npm/cli/issues/4828 | ||
# RUN npm ci | ||
RUN npm install --no-package-lock |
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.
Why this workaround? Are there platform specific optional dependencies in the inspector that need to be pulled into the image for running?
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.
Should probably be npm ci
instead.
Hi, we may need to change also the listening host, now it's hardcoded as 127.0.0.1 that is not exposed in the docker network. a new env variable like CLIENT_HOST and SERVER_HOST should be added. |
Co-authored-by: John McBride <[email protected]>
Hey @slimslenderslacks and @aaronpowell! Can we determine what the exact gap is with this? There is already another PR #471 looking to add this. We have at least been able to test this on Aaron's deployment, so I'm inclined to want this one to make it in. In addition to addressing the comments above by @jpmcb I think we need to set up some Github secrets that @dsp-ant, @ihrpr, @jspahrsummers or other core team members could wrangle for us. |
@cliffhall I've updated the Dockerfile based on the last round of feedback, but I will admit that it is a little clunky to use the container due to the use of environment variables. What I was intending to do was to have it so you could just run The result of this is that you run Admittedly, this is only a problem if you're mapping a non-default port, as |
Thanks, please consider a reverse proxy scenario (i.e. traefik) where a path prefix may be added. I can provide a docker compose example if needed. |
@aaronpowell Is there an updated image I can pull? I see the SHA digest still matches what was there before. ![]() |
with: | ||
registry: ghcr.io | ||
username: ${{ github.actor }} | ||
password: ${{ secrets.GITHUB_TOKEN }} |
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.
That secret is provided by GitHub as part of Actions, it doesn't need to be setup
@aaronpowell Nice, thanks. I'm trying to determine what the gap is so we can commit and try this.
|
@cliffhall here's the docs that describe how to publish to GitHub Container Registry (there is also a section there on publishing to Docker Hub). From an auth perspective, it uses the
The name of the image will be The action only runs on the creation of a release (per this condition) and for my testing I was just creating tags/releases with random names, but using your release structure you'd end up with |
The workflow step I added here is pretty simplistic in terms of what it generates and publishes, but you can add more registries, tags and metadata if you desire. This is an example of a more complex image build that I use on another project that generates a lot more tags and supports multiple container registries (GitHub Container Registry and Azure Container Registry). |
Right. I think we might want to publish to Docker Hub as well, but we'll need to set up a namespace and login info that goes into secrets, etc. For the first go, since this should work with an admin's approval and no further changes, I think this looks pretty good. |
- Need to do a patch level release in order to test the ghcr docker container publishing feature added to main.yml in modelcontextprotocol#257
Fixes #237
Motivation and Context
I want to be able to use a container version of the inspector rather than via npx.
How Has This Been Tested?
I merged the branch into
main
on my fork and created a tag so I could publish a release. Here's the workflow run (ignore the failingpackage
job, I can't publish to npm) and here's the published imageNote: since the workflow is configured to publish to GitHub Packages not Docker Hub the
GITHUB_TOKEN
needs to be able to write packages. This is configured in the workflow, but you also need to update the repo settings to allow the workflow to request a token with write permissions:Breaking Changes
Types of changes
Checklist
Additional context
Currently, the image only has the nodejs runtime in it, so it doesn't support running stdio servers with python/.NET/Java/etc. Instead, they would probably be handled as different tags. My primary focus is on supporthing servers that are SSE (and streamable http in the future)