Skip to content

Docker version#14

Merged
aborruso merged 20 commits into
ondata:mainfrom
piersoft:main
Mar 6, 2026
Merged

Docker version#14
aborruso merged 20 commits into
ondata:mainfrom
piersoft:main

Conversation

@piersoft

@piersoft piersoft commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

Description

Added Dockerfile and docker-compose.yml

Related Issue

Feature for enable installation with: docker compose up --build -d

@aborruso

aborruso commented Mar 5, 2026

Copy link
Copy Markdown
Member

Hi @piersoft, thanks for this contribution — Docker support is a great addition and the multi-stage build with non-root user is well done.

A few things to address before merging:

Language: The project uses English as its primary language. Could you translate DOCKER-README.md to English? We'd also suggest placing it under docs/DOCKER.md to keep the root clean.

Healthcheck (flagged by AI code review): The current healthcheck tries /health first, but that endpoint only exists in the Cloudflare Workers transport, not in the Node.js HTTP transport used by the container. The fallback on ping is also not a valid MCP method. A reliable alternative would be:

HEALTHCHECK --interval=30s --timeout=10s --start-period=15s --retries=3 \
  CMD curl -sf -X POST http://localhost:${PORT}/mcp \
    -H 'Content-Type: application/json' \
    -d '{"jsonrpc":"2.0","method":"tools/list","id":1}' > /dev/null || exit 1

Repo URL: DOCKER-README.md references piersoft/ckan-mcp-server — wouldn't it make more sense to point to ondata/ckan-mcp-server since that's the canonical repo?

Once these are fixed we're happy to merge. Thanks again!

@aborruso

aborruso commented Mar 5, 2026

Copy link
Copy Markdown
Member

Thanks for the updates — language and URL are now good.

Two remaining points:

Dockerfile healthcheck: still using the broken /health + ping fallback. The docker-compose.yml version is correct though, so we'll accept it as-is and open a follow-up issue to align the Dockerfile.

src/portals.json: the diff removes 8 portal entries (data.gov.uk, catalog.data.gov, open.canada.ca, data.gov.au, opendata.swiss, data.stadt-zuerich.ch, govdata.de, dati.anticorruzione.it). There's no reason to remove these from the central repo — they're intentional and used by the community.

If your fork has local customizations, the cleanest way to stay aligned with upstream is:

git remote add upstream https://github.com/ondata/ckan-mcp-server.git
git fetch upstream
git merge upstream/main

This way you can keep your local changes (e.g. removed portals, custom config) in your fork, and when you want to open a PR toward ondata you cherry-pick or revert only the commits that make sense to contribute — without carrying over local-only differences.

Please revert src/portals.json to its original state and we'll merge.

@aborruso

aborruso commented Mar 5, 2026

Copy link
Copy Markdown
Member

Hi @piersoft, the new additions are interesting — an Ollama + React chat UI on top of the MCP server is a real-world use case worth showcasing.

That said, files like App.jsx and ckan-mcp-bridge.js in the repo root don't fit the core project structure. We'd suggest reorganizing everything under an examples/ folder:

examples/
└── ollama-chat/
    ├── App.jsx
    ├── ckan-mcp-bridge.js
    ├── docker-compose.yml
    └── README.md

This way the MCP server core stays clean, and the Ollama+chat stack lives as a self-contained example — a pattern used by many open source projects to show real integrations without polluting the main codebase.

The Dockerfile and base docker-compose.yml for the server itself can stay in the root as originally proposed.

One thing still pending: please revert src/portals.json to its original state (the 8 removed portals need to come back).

If you reorganize along these lines, we're happy to merge the whole thing together. What do you think?

@aborruso

aborruso commented Mar 5, 2026

Copy link
Copy Markdown
Member

Thank you so much @piersoftApp.jsx, index.css and portals.json are all sorted. Almost there!

One last thing: ckan-mcp-bridge.js is still in the root — it should move to examples/ollama-chat/.

Once that's done, we're ready to merge.

@aborruso
aborruso merged commit 66837e3 into ondata:main Mar 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants