Guard conf write and address Copilot copy-edits - #384
Merged
Conversation
Copilot's third review on #381 raised six legitimate findings. Addressing the substantive ones in this PR; remaining one is a PR description fix. - Docker/entrypoint.sh: guard the mediaserver.conf append against a read-only or non-writable bind-mounted etc directory. Failure used to be silent (overall `set -e` is not on, and `>>` to an unwritable file just errors out without aborting the script), leaving the OS variant override missing. Now logs a clear stderr warning naming the required ownership. - README.md: capitalize "Unix" in four "Unix socket" comments (proper-noun consistency). - README.md: replace time-relative "recently disabled" with "disabled in late 2025" so the note ages better. Copilot's seventh finding — that PR #381's test plan still references a build-time verification (`docker run --rm --entrypoint=cat`) that no longer applies after the runtime-injection refactor — will be addressed by editing #381's description directly.
There was a problem hiding this comment.
Pull request overview
This PR is a small follow-up cleanup that improves runtime robustness of the non-LSIO container entrypoint (when appending currentOsVariantOverride=docker into a bind-mounted mediaserver.conf) and applies a couple of README copy-edits for wording consistency.
Changes:
- Guard the
mediaserver.confappend inDocker/entrypoint.shand emit a clearer stderr warning when the bind-mounted config directory isn’t writable by the unprivileged container user. - Copy-edit README tmpfs comments to capitalize “Unix” in “Unix socket”.
- Replace time-relative wording (“recently disabled”) with a more stable phrase (“disabled … in late 2025”).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Docker/entrypoint.sh | Adds conditional write + warning around appending currentOsVariantOverride=docker at container start. |
| README.md | Minor wording/capitalization tweaks in compose examples and licensing note. |
Copilot's review caught that `>> "${FILE}" 2>/dev/null` doesn't
suppress the open-failure error: bash sets up redirections
left-to-right, so the `>>` opens (or fails) before stderr is
redirected. Drop the redundant `2>/dev/null` — if the open fails,
bash's natural "permission denied" message and our explicit warning
both go to the user, which is more informative than the silent-fail
intent of the misplaced redirect.
This was referenced May 25, 2026
Merged
ptr727
added a commit
that referenced
this pull request
May 25, 2026
## Summary Round-4 follow-up from Copilot's review on PR #381. The LSIO init script ([`init-nx-relocate/run`](Docker/s6-overlay/s6-rc.d/init-nx-relocate/run)) appended to `/config/etc/mediaserver.conf` without checking the result. The script runs as root, but the underlying `/config` mount can be on a read-only filesystem or otherwise rejected, so a silent failure would leave mediaserver running without the OS variant override. ## Fix Apply the same defensive pattern non-LSIO already uses in [`Docker/entrypoint.sh`](Docker/entrypoint.sh) (added in #384): wrap the append in an `if`, log a clear stderr warning on failure, and only `chown` on successful write. Both variants now log "Added" on success and "Warning: failed to write" on failure with a hint about what to check. ## Test plan - [x] `dotnet test CreateMatrixTests/CreateMatrixTests.csproj` — 16/16 pass. - [ ] CI matrix passes.
ptr727
added a commit
that referenced
this pull request
May 25, 2026
## Summary Rolls up commits from develop. Brings the upstream-aligned `currentOsVariantOverride=docker` runtime injection, the README `ignoreRootTool` deviation note, refreshed compose examples (tmpfs `/tmp`, `/dev/dri` passthrough, storage-pool comments), and dependency bumps onto main. ## Why so many commits Initial #380 attempted a build-time RUN that worked for fresh installs but Copilot caught two bind-mount problems: - **LSIO** — `init-nx-relocate` replaces `/opt/.../mediaserver/etc` with a symlink to `/config/etc` on first start, erasing the build-time edit. Fixed in #380 round-2 by moving injection into the s6 init script. - **Non-LSIO** — the README's recommended setup bind-mounts `/opt/.../mediaserver/etc` from the host, hiding the build-time edit. Fixed in #382 by moving injection into `entrypoint.sh`. Subsequent iterations cleaned up the idempotence check (#383 — match the key alone, not the value) and made the write robust against non-writable bind mounts (#384 — guard the redirect, log a clear warning). ## Visible behavior changes for main - `mediaserver.conf` gets `currentOsVariantOverride=docker` appended on **container start** if the key is not already present. Injection happens in `entrypoint.sh` for non-LSIO and `init-nx-relocate/run` for LSIO. Match is key-only (`^currentOsVariantOverride=`) so a user-set value is never overridden and never duplicated. - README's Known Issues > Licensing now documents that upstream's `ignoreRootTool=true` deviation is **deliberately not adopted**. - Compose examples include `tmpfs: /tmp:size=1g,mode=1777` for RAM-backed temp files / Unix socket; production example shows `/dev/dri` iGPU passthrough and storage-pool layout. - Dependabot bumps (nuget-deps, actions-deps) included. ## Test plan The injection now happens on container start, so you must let the entrypoint run — `docker run --rm --entrypoint=cat ...` will skip it and produce a false negative. - [ ] Post-merge push to main fires `publish-release.yml` automatically. - [ ] Non-LSIO: start a container with the README compose example, then `docker exec <container> cat /opt/networkoptix/mediaserver/etc/mediaserver.conf | grep currentOsVariantOverride` shows the line. - [ ] LSIO: start with an empty `/config` volume, then `docker exec <container> cat /config/etc/mediaserver.conf | grep currentOsVariantOverride` shows the line. - [ ] Restart either container and confirm the line is not duplicated (idempotence). - [ ] Pre-populate `mediaserver.conf` with `currentOsVariantOverride=something-else`, start the container, confirm the existing value is preserved (no override). - [ ] Bind-mount a read-only etc directory in non-LSIO, start the container, confirm the entrypoint logs the "failed to write" warning to stderr and mediaserver still starts.
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
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.
Summary
Round-3 follow-up from Copilot's review on PR #381 (develop -> main sync). Six findings; this PR addresses the substantive ones; the seventh (PR #381's test plan referencing a build-time verification that no longer applies) will be fixed by editing #381's description directly.
Changes
Docker/entrypoint.sh— guard themediaserver.confappend. The entrypoint runs as the unprivileged${COMPANY_NAME}user, so the bind-mounted etc directory must be writable by that user. Previously the>>would silently error if not writable. Now wraps the redirect in a conditional and logs a clear stderr warning identifying the required ownership.README.md(4 lines) — capitalize "Unix" in "Unix socket" tmpfs comments. Proper-noun consistency.README.md(1 line) — replace "recently disabledroot-tool" with "disabledroot-toolin late 2025". Less time-relative, ages better.Test plan
dotnet test CreateMatrixTests/CreateMatrixTests.csproj— 16/16 pass.