Skip to content

add Docker compose - #7

Open
abhilashingale wants to merge 1 commit into
synapticon:mainfrom
abhilashingale:add-docker-compose
Open

abhilashingale wants to merge 1 commit into
synapticon:mainfrom
abhilashingale:add-docker-compose

Conversation

@abhilashingale

Copy link
Copy Markdown

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.

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>
@stefanbogdanov2c
stefanbogdanov2c self-requested a review September 15, 2026 13:15
@stefanbogdanov2c

Copy link
Copy Markdown
Collaborator

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.

  1. The bind mount hides the image's dist/, so a documented command fails (blocking)

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.

  1. The claim about .env is wrong in both directions (README:173)

▎ "A .env file in the repository root is deliberately not copied into the image, so pass the hostname this way instead."

  • .dockerignore only affects the build context. At runtime the repo is bind-mounted, so .env is present inside the container at /usr/src/app/.env, and dotenv reads it in src/init-client.ts:1.
  • More importantly: Compose itself auto-loads .env from the project directory for interpolation, so ${MOTION_MASTER_HOSTNAME:-host.docker.internal} (docker-compose.yml:46) already picks up the value from .env. The workflow described at README:47 keeps working; the Docker section says it doesn't.

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.

  1. The node_modules volume goes stale after a dependency change (README:181)

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

  • npm install ignores the lockfile (Dockerfile:32). package-lock.json already arrives via COPY package*.json ./; npm ci gives a reproducible and faster build.
  • restart: unless-stopped on a tail -f /dev/null container (docker-compose.yml:23) means a dev container that comes back on every daemon restart and at boot, indefinitely. Fine if deliberate, but worth a conscious decision; the default (no) is less surprising.
  • The docker commit advice promises more than it delivers (README:160). Both things worth preserving live outside the container layer (sources → bind mount, packages → volume), so the commit captures a near-empty layer. The table just below says as much — either drop the suggestion or spell out what it does not capture.
  • .dockerignore and .gitignore disagree about test-motion-master-macOS/. Git ignores the whole directory; .dockerignore excludes only .csv/.png from it, so the scratch .ts scripts still enter the build context. Harmless now that include exists, but it contradicts the "108 MB of pointless build context" comment. Exclude the whole directory.
  • The docs speak exclusively in macOS terms ("the Mac", "edit on the Mac" — docker-compose.yml:29, Dockerfile:5-8, README:106) in a repo that isn't macOS-specific, while extra_hosts already covers Linux correctly. Host-agnostic phrasing with the macOS specifics as a note would read better. Likewise, the comments in .gitignore/.dockerignore reference a directory that doesn't exist in the repo, which will puzzle another reader.
  • ROARR_LOG: "${ROARR_LOG:-}" (docker-compose.yml:47) sets the variable to an empty string rather than leaving it unset. Harmless with roarr (it checks for === 'true'), but if you want a clean pass-through, list form with a bare key (- ROARR_LOG) omits it when undefined.

I'd fix 1 (doesn't work) and 2 (actively misleads the reader) before merging. The rest is safe as follow-up.

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