Correct docker/README.md: documented volumes and services that do not exist - #2
Open
ShreeBohara wants to merge 1 commit into
Open
Correct docker/README.md: documented volumes and services that do not exist#2ShreeBohara wants to merge 1 commit into
ShreeBohara wants to merge 1 commit into
Conversation
…ot exist Every claim below was checked against docker/docker-compose.yml and config.py. - Ollama from inside a container: OLLAMA_BASE_URL defaults to http://localhost:11434, which inside the api container is the container itself, not the host. This became reachable only now that compose forwards the variable (previously the Ollama path was unusable under Docker at all), so document host.docker.internal plus the extra_hosts line Linux needs. - Data Persistence was wrong in both mechanism and names. It claimed named volumes `api-data` and `repos-data`; neither exists anywhere in the compose file. The api service bind-mounts ../data:/app/data, so state lives in the working tree and survives `compose down`. Noted the root-owned-files consequence and the declared but never mounted `data:` volume. - Services omitted redis, which the file defines and which api declares depends_on, so it is not optional. Flagged that it is published unauthenticated. - `docker-compose up` -> `docker compose up`; the v1 binary is end-of-life. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1. Documentation only — no code changes.
While adding one note I'd flagged, I read the rest of the file and found two more claims that were simply false. Every statement in the new version was verified against
docker-compose.ymlandconfig.py.1. Ollama could not work inside a container, and still can't without this
OLLAMA_BASE_URLdefaults tohttp://localhost:11434. Inside theapicontainer that is the container, not your host, so an Ollama setup silently fails to connect.This is worth documenting now specifically because #1 made it reachable: compose previously never forwarded
OLLAMA_BASE_URL, so the Ollama path was unusable under Docker regardless. Fixing that turned a dead path into a confusing one. Now documented withhost.docker.internaland theextra_hosts: host-gatewayline Linux additionally needs.2. "Data Persistence" was wrong in both mechanism and names
It claimed:
Neither volume exists anywhere in the compose file. What actually happens is a bind mount:
So the SQLite DB, ChromaDB and all cloned repos live in
data/in your working tree and survivedocker compose down— the opposite of the isolation the old text implied. Also documented two real consequences: the container runs as root so those files come out root-owned on the host, and thedata:named volume the compose file declares is never mounted and has no effect.3.
Servicesomitted redisThe compose file defines
redisandapideclaresdepends_on: redis, so it is not optional. Added it, and noted it is published on 6379 with no password — fine locally, not on a shared host.4.
docker-compose up→docker compose upThe v1 binary is end-of-life.
Verification
All ten factual claims in the new text were asserted programmatically against the source files (services present, bind-mount path, redis port,
depends_on, orphan volume, absence ofapi-data/repos-data,OLLAMA_BASE_URLforwarded, and both config defaults). No remaining v1docker-composeinvocations in tracked docs or scripts.Still unverified, and I want to be explicit: I could not build or run these images — Docker is not installed on the machine I worked on. This PR corrects documentation against the compose file and config; it is not a report of a successful
docker compose up.🤖 Generated with Claude Code