Update Dockerfile, workflows, and documentation for clarity and compatibility - #2
Conversation
…pdate Node.js version requirements
- Modify server.yaml for seq-otel configuration and metadata - Enhance tools.json with input schemas for better validation - Add seq-otel icon to assets and update README for catalog compatibility - Create docker-mcp-registry-submission checklist for PR preparation
There was a problem hiding this comment.
Pull request overview
This PR updates the project’s packaging, catalog metadata, and CI workflows to better align with Docker MCP Registry submission expectations, improve documentation clarity, and standardize repository ownership/configuration.
Changes:
- Update Node.js requirements and introduce
.nvmrc-driven Node setup in new GitHub workflows. - Refresh Docker MCP catalog metadata (
catalog/server.yaml,catalog/tools.json) and add a registry submission checklist. - Adjust container/build metadata and repository ownership information (Dockerfile labels, CODEOWNERS), plus add a public icon asset.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Raises Node engine requirement to >=22.0.0. |
| docs/docker-mcp-registry-submission.md | Adds a submission checklist targeting docker/mcp-registry. |
| catalog/tools.json | Expands tool metadata with inputSchema for selected tools. |
| catalog/server.yaml | Updates server identity/image reference and config schema/env mapping for registry consumption. |
| assets/seq-otel-icon.svg | Adds an icon asset referenced by the catalog metadata. |
| README.md | Clarifies Node requirement reference and adds registry submission checklist link. |
| Dockerfile | Switches to node:lts-alpine, updates OCI labels, keeps non-root runtime user. |
| CODEOWNERS | Simplifies ownership to a repository-wide owner and key paths. |
| .nvmrc | Adds Node version selection for dev/CI via lts/*. |
| .github/workflows/docker-build-validate.yml | Adds CI workflow to typecheck, build, build image, and validate container startup contract. |
| .github/workflows/copilot-setup-steps.yml | Adds CI workflow to validate build plus required Copilot/project documentation artifacts. |
You can also share your feedback on Copilot code review. Take the survey.
| @@ -10,18 +10,18 @@ COPY tsconfig.json ./ | |||
| COPY src ./src | |||
| RUN npm run build | |||
|
|
|||
| FROM node:20-alpine AS runtime | |||
| FROM node:lts-alpine AS runtime | |||
There was a problem hiding this comment.
Using the floating node:lts-alpine tag makes builds non-reproducible and can introduce breaking changes unexpectedly when the LTS line moves. Pin the base image to a specific Node major/minor (and ideally a digest), and keep build/runtime stages aligned with the Node version required by package.json.
| @@ -0,0 +1 @@ | |||
| lts/* | |||
There was a problem hiding this comment.
lts/* makes local/CI Node versions drift over time and can cause unexpected breakages (or subtle behavior changes) when the active LTS line changes. Consider pinning to a specific major (e.g., 22) to match the package.json engines requirement and keep builds deterministic.
| lts/* | |
| 22 |
| }, | ||
| "engines": { | ||
| "node": ">=20" | ||
| "node": ">=22.0.0" |
There was a problem hiding this comment.
Raising the engine requirement to >=22.0.0 is fine, but please ensure the Docker build/runtime Node version and CI Node version are pinned consistently (instead of floating lts/* / node:lts-*) so the project doesn’t end up with mismatched or non-reproducible environments.
| "node": ">=22.0.0" | |
| "node": "22.13.0" |
| - name: Setup Node | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version-file: ".nvmrc" |
There was a problem hiding this comment.
This workflow relies on .nvmrc for the Node version; with .nvmrc set to lts/*, CI will change behavior over time as the LTS line advances. Prefer pinning the Node major (or using an explicit node-version) so the build/validation remains deterministic and matches the declared engines requirement.
| node-version-file: ".nvmrc" | |
| node-version: "20.x" |
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version-file: ".nvmrc" |
There was a problem hiding this comment.
Using node-version-file: .nvmrc is good for consistency, but with .nvmrc set to lts/* the Node version (and thus build output) will drift over time. Consider pinning the Node major in .nvmrc (or setting node-version explicitly) to keep this workflow stable.
| node-version-file: ".nvmrc" | |
| node-version: "20" |
| }, | ||
| "engines": { | ||
| "node": ">=20" | ||
| "node": ">=22.0.0" |
There was a problem hiding this comment.
package-lock.json still records the previous engines constraint (>=20) under packages[""].engines. After changing the Node engine in package.json, consider regenerating/updating the lockfile so the metadata stays consistent (helps avoid confusion in tooling that reads engines from the lockfile).
| "node": ">=22.0.0" | |
| "node": ">=20.0.0" |
| name: seq-mcp | ||
| image: seq-mcp:local | ||
| name: seq-otel | ||
| image: mcp/seq-otel:latest |
There was a problem hiding this comment.
Using an image tag of :latest makes deployments non-reproducible and complicates rollback/auditing. Prefer publishing and referencing an immutable, versioned tag (and/or digest) in the catalog metadata, reserving latest for development flows if needed.
| image: mcp/seq-otel:latest | |
| image: mcp/seq-otel:v1 |
Enhance project documentation and workflows, update Dockerfile for better compatibility, and clarify ownership in CODEOWNERS. Add a submission checklist for Docker MCP Registry and improve validation schemas. Update Node.js version requirements and ensure proper setup for GitHub workflows.