add Docker compose - #7
abhilashingale wants to merge 1 commit into
Conversation
Adds a docker-compose.yml defining a long-lived "motion-master-client" container with the repository bind-mounted, so scripts are edited on the host and run inside the container. node_modules stays in a named volume, otherwise the mount covers the image's Linux dependencies with the host's. Also fixes what stopped the container path working at all: - Dockerfile ran "npm run build" before "COPY . .", so tsc had no tsconfig.json or src/ and the build always failed. Sources are now copied first. - tsconfig.json had no "include", so tsc globbed '**/*' and failed with TS6059 on any .ts file outside rootDir. Restricted to src/, which also repairs "npm run build" on the host. - Added .dockerignore: without it "COPY . ." pulled in the host's platform-specific node_modules and shadowed the image's own. README gains a "Running in Docker" section (up, exec, stop, start, down, commit), a note that MOTION_MASTER_HOSTNAME must be host.docker.internal rather than localhost inside a container, and an nvm hint for when npx is missing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed the full diff against main (6 files, 1 commit). Overall this is solid, and the comments are unusually good — they explain why rather than what. The reordering in the Dockerfile is a genuine fix: the previous build was broken (RUN npm run build ran before COPY . ., so tsc failed with TS18003: No inputs were found in config file). include: ["src"] in tsconfig.json, the named volume for node_modules, and extra_hosts: host-gateway are all the right patterns. That said, there's one thing that doesn't actually work, and two inaccuracies in the docs.
docker-compose.yml:32 mounts .:/usr/src/app over the entire working directory. That completely shadows /usr/src/app/dist, which RUN npm run build produced in the image, with the host's working tree. On a clean clone there is no dist/ on the host (it's in .gitignore), so: docker compose exec client node dist/request/get-devices.promise.js # README:131 fails with Cannot find module. This passed during testing because dist/ already existed from a local build on that machine. It also undercuts the argument at README:22 ("the image brings its own Node") — you'd have to build on the host first after all. Simplest fix: document a build inside the container as the first step (docker compose exec client npm run build — via the bind mount the output lands straight in the working tree, which is actually convenient), or drop that section. The RUN npm run build in the Dockerfile isn't wasted either way: it still serves the plain docker run path documented in the Dockerfile header, where there is no bind mount.
▎ "A .env file in the repository root is deliberately not copied into the image, so pass the hostname this way instead."
The real trap, which the README doesn't mention: Compose always sets MOTION_MASTER_HOSTNAME in the container environment, and dotenv does not override already-set variables by default. When the two disagree — e.g. you edit .env and then run docker compose start instead of up — the value baked in at up time wins, and the .env edit appears to have no effect. That's what's worth documenting in place of the current sentence.
A named volume is seeded from the image only when it is first created. Change package.json → docker compose build → up -d, and the container still has the old packages, with no warning. The comment at docker-compose.yml:36 states the seeding rule correctly, but the README table sells the persistence as an unqualified win. Worth a line: after changing dependencies, either docker compose exec client npm install, or docker compose down -v && docker compose up -d --build. Minor
I'd fix 1 (doesn't work) and 2 (actively misleads the reader) before merging. The rest is safe as follow-up. |
Adds a docker-compose.yml defining a long-lived "motion-master-client" container with the repository bind-mounted, so scripts are edited on the host and run inside the container. node_modules stays in a named volume, otherwise the mount covers the image's Linux dependencies with the host's.
Also fixes what stopped the container path working at all:
README gains a "Running in Docker" section (up, exec, stop, start, down, commit), a note that MOTION_MASTER_HOSTNAME must be host.docker.internal rather than localhost inside a container, and an nvm hint for when npx is missing.