Add a github workflow to build and test the container#81
Conversation
Fix CI: build base image from ./base/amd64 before building node image Updated dockerfile Use a self-contained approach Add a container startup test Update README.md to match current code: self-contained Dockerfile, correct env var names, node/ compose path, boto3 instead of rsync/s3fs Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
📝 WalkthroughWalkthroughAdds a container CI workflow (build/test/push), introduces a scoped Node Docker image and Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub (Push/PR/Tag/Dispatch)
participant Test as Test Job
participant Build as Build-and-Push Job
participant Container as Container (build/run)
participant GHCR as GHCR Registry
GH->>Test: Trigger workflow (push/PR/tag/dispatch)
Test->>Container: Build image from `node/Dockerfile`
Container->>Container: Run verification (Python, FFmpeg, JACK, deps, `/app/stream.sh`)
Test->>Container: Create `test.env`, start container detached
Container-->>Test: Return status / exit code
alt status running OR exit code 0
Test-->>Build: proceed to build-and-push
Build->>Build: Setup QEMU & Buildx, generate tags/labels
Build->>GHCR: Build & push multi-arch images
else
Test-->>GH: fail workflow
end
sequenceDiagram
participant Init as stream.sh Init
participant Env as Environment Vars
participant Sys as Host Capability
participant JACK as jackd
participant FF as ffmpeg/live-stream
Init->>Env: Check STREAM_RATE / SAMPLE_RATE
alt unset
Env->>Env: set defaults and log
end
Init->>Sys: run `nice -n -1 true`
alt succeeds (CAP_SYS_NICE)
Sys->>Init: set NICE_HIGH, NICE_MED, JACKD_RT="-P 75"
else
Sys->>Init: leave NICE/JACKD_RT empty, log notice
end
Init->>JACK: start `jackd` with conditional $JACKD_RT
Init->>FF: start ffmpeg/live-stream using $NICE_HIGH/$NICE_MED
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the container build/deploy flow by introducing a GitHub Actions workflow that builds/tests (and on main/tags pushes) a self-contained node container image, while aligning documentation and configuration templates with the current runtime behavior.
Changes:
- Add a GitHub Actions workflow to build, smoke-test, and publish a multi-arch container image to GHCR.
- Replace the previous “base image” approach with a self-contained
node/Dockerfileand update runtime scripts for env defaults/priority handling. - Update docs and templates: README refresh, add
node/.env.example, and extend.gitignoreentries.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
node/stream.sh |
Adds env defaults, safer string comparisons, and conditional process priority handling. |
node/Dockerfile |
Switches to a self-contained python-slim image with OS + pip dependencies and runs stream.sh. |
node/.gitignore |
Ignores secrets .env while keeping .env.example tracked. |
node/.env.example |
Adds a copy-ready configuration template for deployments. |
base/upload_s3.py |
Fixes logging formatting for bucket debug output. |
base/upload_flac_s3.py |
Fixes logging formatting for bucket debug output. |
README.md |
Updates setup/run instructions and dependency descriptions to match current architecture. |
.gitignore |
Adds .vs ignore (and keeps .env ignored). |
.github/workflows/build-container.yml |
Adds CI build/test and main/tag build-and-push to GHCR with caching and metadata tagging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
node/stream.sh (1)
57-59: Make thelimits.confedits idempotent.Every restart appends the same two lines again, so long-lived containers accumulate duplicates in
/etc/security/limits.conf. Guard the writes so the startup path stays stable across restarts.Suggested shell change
-echo `@audio` - memlock 256000 >> /etc/security/limits.conf -echo `@audio` - rtprio 75 >> /etc/security/limits.conf +grep -qxF '@audio - memlock 256000' /etc/security/limits.conf || echo '@audio - memlock 256000' >> /etc/security/limits.conf +grep -qxF '@audio - rtprio 75' /etc/security/limits.conf || echo '@audio - rtprio 75' >> /etc/security/limits.conf🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/stream.sh` around lines 57 - 59, The two echo lines appending "@audio - memlock 256000" and "@audio - rtprio 75" to /etc/security/limits.conf are not idempotent and will duplicate on every restart; update the startup script (node/stream.sh) to first check for the exact lines (e.g., using grep -qF or grep -qxF) and only append each line if it is not already present, so the entries are added once and not duplicated across restarts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@node/.env.example`:
- Around line 15-18: The .env.example advertises BUCKET_TYPE but omits the
required variables read by base/upload_s3.py and base/upload_flac_s3.py; add
BUCKET_STREAMING and BUCKET_ARCHIVE entries to the template (with placeholder
values and a short comment) so when BUCKET_TYPE=custom the container won't fail
due to missing env vars—include both variable names (BUCKET_STREAMING,
BUCKET_ARCHIVE) near the BUCKET_TYPE section and show example placeholders for
dev/prod/custom usage.
- Around line 20-23: The .env.example lists NODE_TYPE=offline even though
node/stream.sh does not handle "offline"; remove or comment out the "offline"
option in node/.env.example (leave only supported values like hls-only,
research, debug, dev-virt-s3) and update the accompanying comment to warn not to
enable unsupported modes; only re-add "offline" once node/stream.sh implements
branching for NODE_TYPE "offline".
In `@node/Dockerfile`:
- Around line 30-35: The Dockerfile's COPY node/ ./ instruction will include
sensitive files like node/.env into the image; add a root .dockerignore (placed
at repository root) to exclude deployment secrets and unnecessary files (e.g.,
node/.env, base/*.env, .git, node/node_modules, **/*.pem, **/*.key, .env) so
those files are not sent to the Docker daemon or baked into image layers; ensure
the .dockerignore is committed and then rebuild to verify COPY node/ ./ no
longer includes excluded files.
---
Nitpick comments:
In `@node/stream.sh`:
- Around line 57-59: The two echo lines appending "@audio - memlock 256000" and
"@audio - rtprio 75" to /etc/security/limits.conf are not idempotent and will
duplicate on every restart; update the startup script (node/stream.sh) to first
check for the exact lines (e.g., using grep -qF or grep -qxF) and only append
each line if it is not already present, so the entries are added once and not
duplicated across restarts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bbcd81a-266e-472f-96d7-9fa63160f548
📒 Files selected for processing (9)
.github/workflows/build-container.yml.gitignoreREADME.mdbase/upload_flac_s3.pybase/upload_s3.pynode/.env.examplenode/.gitignorenode/Dockerfilenode/stream.sh
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
node/Dockerfile (1)
30-31:⚠️ Potential issue | 🟠 MajorKeep deployment secrets out of the image build context.
Line 31 still copies the whole
node/directory, sonode/.envwill be baked into the image if a root.dockerignoredoes not exclude it..gitignoredoes not protect Docker builds.Suggested `.dockerignore`
+# Keep deployment secrets out of Docker build context +node/.env +!node/.env.example🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/Dockerfile` around lines 30 - 31, The Dockerfile currently uses COPY node/ ./ which can bake deployment secrets like node/.env into the image; stop including secrets by adding a .dockerignore that excludes files such as .env, .env.* and other secret/config files, and ensure the build context does not contain secrets before running the build; alternatively change the Dockerfile workflow to COPY only the required artifacts (e.g., package.json, package-lock.json, src/) instead of copying the whole directory (refer to the COPY node/ ./ instruction) so secrets are not included in the image.node/.env.example (1)
15-18:⚠️ Potential issue | 🟠 MajorAdd the missing custom-bucket variables to the template.
This still advertises
BUCKET_TYPE=customin the file docs, but the template never definesBUCKET_STREAMINGorBUCKET_ARCHIVE. Copy-pastingcustomwill fail at startup when the upload scripts read those env vars.Suggested template update
# === S3 Bucket Selection === BUCKET_TYPE=dev `#BUCKET_TYPE`=prod +#BUCKET_TYPE=custom +#BUCKET_STREAMING=your-streaming-bucket +#BUCKET_ARCHIVE=your-archive-bucket🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/.env.example` around lines 15 - 18, The template advertises a custom BUCKET_TYPE but doesn't define the required env vars, causing startup failures; update the .env template to include the missing BUCKET_STREAMING and BUCKET_ARCHIVE entries (and any related custom-bucket keys the upload scripts expect) alongside the existing BUCKET_TYPE setting so that when BUCKET_TYPE=custom is used the upload/init code can read BUCKET_STREAMING and BUCKET_ARCHIVE (look for BUCKET_TYPE, BUCKET_STREAMING, BUCKET_ARCHIVE in the repo to ensure you add exactly the keys the upload scripts reference).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@node/stream.sh`:
- Around line 56-57: The script currently starts jackd unconditionally
(JACK_NO_AUDIO_RESERVATION=1 jackd ... -d hw:$AUDIO_HW_ID ...) which binds to
real hardware and prevents dev-virt-s3 overrides; change it so jackd (and
subsequent jack_connect calls) only run for hardware-backed NODE_TYPEs by gating
the jackd startup and the later jack_connect block behind a condition that
checks NODE_TYPE or the USE_JACK_INPUT flag. Locate the jackd invocation and the
jack_connect sequence and wrap them in the same conditional (e.g., if [
"$NODE_TYPE" = "dev-hw" ] or if [ "$USE_JACK_INPUT" = "1" ]), ensuring
SAMPLE_RATE/STREAM_RATE overrides for dev-virt-s3 occur before any jackd start
when in virtual mode.
---
Duplicate comments:
In `@node/.env.example`:
- Around line 15-18: The template advertises a custom BUCKET_TYPE but doesn't
define the required env vars, causing startup failures; update the .env template
to include the missing BUCKET_STREAMING and BUCKET_ARCHIVE entries (and any
related custom-bucket keys the upload scripts expect) alongside the existing
BUCKET_TYPE setting so that when BUCKET_TYPE=custom is used the upload/init code
can read BUCKET_STREAMING and BUCKET_ARCHIVE (look for BUCKET_TYPE,
BUCKET_STREAMING, BUCKET_ARCHIVE in the repo to ensure you add exactly the keys
the upload scripts reference).
In `@node/Dockerfile`:
- Around line 30-31: The Dockerfile currently uses COPY node/ ./ which can bake
deployment secrets like node/.env into the image; stop including secrets by
adding a .dockerignore that excludes files such as .env, .env.* and other
secret/config files, and ensure the build context does not contain secrets
before running the build; alternatively change the Dockerfile workflow to COPY
only the required artifacts (e.g., package.json, package-lock.json, src/)
instead of copying the whole directory (refer to the COPY node/ ./ instruction)
so secrets are not included in the image.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 645d84d6-0149-4ef8-9fa3-256d898e389e
📒 Files selected for processing (4)
.github/workflows/build-container.ymlnode/.env.examplenode/Dockerfilenode/stream.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build-container.yml
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
Create a WAV file to test with Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
20226af to
95a646e
Compare
Signed-off-by: Dave Thaler <dthaler1968@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
node/stream.sh (2)
56-60: Quote$AUDIO_HW_IDand$SAMPLE_RATEto prevent word splitting.While
$JACKD_RTis intentionally unquoted (to disappear when empty),$AUDIO_HW_IDand$SAMPLE_RATEshould be quoted to handle edge cases.🔧 Proposed fix
if [ "$NODE_TYPE" != "dev-virt-s3" ]; then echo "Starting Jack audio server for hardware audio capture..." - JACK_NO_AUDIO_RESERVATION=1 jackd -t 2000 $JACKD_RT -d alsa -d hw:$AUDIO_HW_ID -r $SAMPLE_RATE -p 1024 -n 10 -s & + JACK_NO_AUDIO_RESERVATION=1 jackd -t 2000 $JACKD_RT -d alsa -d "hw:$AUDIO_HW_ID" -r "$SAMPLE_RATE" -p 1024 -n 10 -s & fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/stream.sh` around lines 56 - 60, The jackd invocation may suffer word-splitting because $AUDIO_HW_ID and $SAMPLE_RATE are unquoted; update the jackd command in the block that sets JACK_NO_AUDIO_RESERVATION and calls jackd (the line with "JACK_NO_AUDIO_RESERVATION=1 jackd ... -d hw:$AUDIO_HW_ID -r $SAMPLE_RATE ... &") to quote those two expansions (e.g., use -d "hw:$AUDIO_HW_ID" and -r "$SAMPLE_RATE") while leaving $JACKD_RT unquoted so it still disappears when empty.
94-102: Inconsistent indentation indev-virt-s3block.Lines 97 and 100 use 2-space indentation while surrounding lines use 4 spaces. This doesn't affect functionality but impacts readability.
🔧 Proposed fix for consistent indentation
elif [ "$NODE_TYPE" = "dev-virt-s3" ]; then SAMPLE_RATE=48000 STREAM_RATE=48000 - echo "Sampling from virtual WAV file at $SAMPLE_RATE Hz..." + echo "Sampling from virtual WAV file at $SAMPLE_RATE Hz..." echo "Asking ffmpeg to stream only HLS segments at $STREAM_RATE Hz......" ## Streaming HLS only via mpegts - $NICE_HIGH ffmpeg -re -fflags +genpts -stream_loop -1 -i "samples/haro-strait_2005.wav" \ + $NICE_HIGH ffmpeg -re -fflags +genpts -stream_loop -1 -i "samples/haro-strait_2005.wav" \ -f segment -segment_list "/tmp/$NODE_NAME/hls/$timestamp/live.m3u8" -segment_list_flags +live -segment_time $SEGMENT_DURATION -segment_format mpegts \ -ar $STREAM_RATE -ac $CHANNELS -threads 3 -acodec aac "/tmp/$NODE_NAME/hls/$timestamp/live%03d.ts" &🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/stream.sh` around lines 94 - 102, The dev-virt-s3 case has inconsistent indentation: normalize the indentation to match the surrounding 4-space style for the SAMPLE_RATE and STREAM_RATE assignments and the ffmpeg-related echo lines; specifically adjust the two echo lines ("Sampling from virtual WAV..." and "Asking ffmpeg to stream...") and the ffmpeg invocation so they use the same 4-space indent as the other statements in the NODE_TYPE = "dev-virt-s3" block (look for the NODE_TYPE check, SAMPLE_RATE, STREAM_RATE, and the ffmpeg command) to improve readability..github/workflows/build-container.yml (1)
89-128: Thehls-onlytest will always encounter JACK failures in CI.Since
hls-onlymode starts JACK (which requires audio hardware), this test will always fail to produce actual HLS output. The test currently accepts this ("audio hardware errors expected" on line 118), but consider whether this provides meaningful validation beyond verifying the container starts.If the goal is just smoke-testing that the script doesn't crash on startup, this is fine. Otherwise, consider adding a comment clarifying the test's limited scope or skipping verification of HLS output for this mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-container.yml around lines 89 - 128, The hls-only test (NODE_TYPE=hls-only) will always trigger JACK/hardware errors in CI and thus doesn't validate real HLS output; update the test around the docker run for orcanode-test-hw and test.env to either (A) add an explicit comment stating this is only a startup/smoke test and not an HLS functional test (mention NODE_TYPE=hls-only and JACK), or (B) change the verification to only assert the container started/exited cleanly (check STATUS/EXIT_CODE) and skip any HLS/output verification for this run; implement one of these fixes so the intent is clear and the test doesn't attempt to validate hardware-dependent audio output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-container.yml:
- Around line 155-170: Update the docker/metadata-action step (id: meta) to use
a simplified image name under the images key: change images: ghcr.io/${{
github.repository }}/orcanode to images: ghcr.io/${{ github.repository }} so the
resulting image path becomes ghcr.io/orcasound/orcanode instead of the redundant
ghcr.io/orcasound/orcanode/orcanode; keep the rest of the metadata-action
configuration (tags block and type/raw latest enable) unchanged.
---
Nitpick comments:
In @.github/workflows/build-container.yml:
- Around line 89-128: The hls-only test (NODE_TYPE=hls-only) will always trigger
JACK/hardware errors in CI and thus doesn't validate real HLS output; update the
test around the docker run for orcanode-test-hw and test.env to either (A) add
an explicit comment stating this is only a startup/smoke test and not an HLS
functional test (mention NODE_TYPE=hls-only and JACK), or (B) change the
verification to only assert the container started/exited cleanly (check
STATUS/EXIT_CODE) and skip any HLS/output verification for this run; implement
one of these fixes so the intent is clear and the test doesn't attempt to
validate hardware-dependent audio output.
In `@node/stream.sh`:
- Around line 56-60: The jackd invocation may suffer word-splitting because
$AUDIO_HW_ID and $SAMPLE_RATE are unquoted; update the jackd command in the
block that sets JACK_NO_AUDIO_RESERVATION and calls jackd (the line with
"JACK_NO_AUDIO_RESERVATION=1 jackd ... -d hw:$AUDIO_HW_ID -r $SAMPLE_RATE ...
&") to quote those two expansions (e.g., use -d "hw:$AUDIO_HW_ID" and -r
"$SAMPLE_RATE") while leaving $JACKD_RT unquoted so it still disappears when
empty.
- Around line 94-102: The dev-virt-s3 case has inconsistent indentation:
normalize the indentation to match the surrounding 4-space style for the
SAMPLE_RATE and STREAM_RATE assignments and the ffmpeg-related echo lines;
specifically adjust the two echo lines ("Sampling from virtual WAV..." and
"Asking ffmpeg to stream...") and the ffmpeg invocation so they use the same
4-space indent as the other statements in the NODE_TYPE = "dev-virt-s3" block
(look for the NODE_TYPE check, SAMPLE_RATE, STREAM_RATE, and the ffmpeg command)
to improve readability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d786679-465e-4fa7-b2f5-dee010a60162
📒 Files selected for processing (3)
.dockerignore.github/workflows/build-container.ymlnode/stream.sh
✅ Files skipped from review due to trivial changes (1)
- .dockerignore
| - name: Extract metadata | ||
| id: meta | ||
| uses: docker/metadata-action@8e5442c4ef9f78752691e2d8f8d19755c6f78e81 # v5.5.1 | ||
| with: | ||
| images: ghcr.io/${{ github.repository }}/orcanode | ||
| tags: | | ||
| # Semver tags on release | ||
| type=semver,pattern={{version}} | ||
| type=semver,pattern={{major}}.{{minor}} | ||
| type=semver,pattern={{major}} | ||
| type=ref,event=tag | ||
| # Branch-based tags | ||
| type=ref,event=branch | ||
| type=sha | ||
| # Latest tag for main branch | ||
| type=raw,value=latest,enable={{is_default_branch}} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the repository name to confirm the resulting image path
echo "Repository: orcasound/orcanode"
echo "Resulting image path: ghcr.io/orcasound/orcanode/orcanode"
echo ""
echo "If this is unintended, the image path should be: ghcr.io/orcasound/orcanode"Repository: orcasound/orcanode
Length of output: 227
🏁 Script executed:
# Check if there are other container images or references in the workflow
rg "images:" .github/workflows/ -A 2Repository: orcasound/orcanode
Length of output: 290
🏁 Script executed:
# Look for any documentation about container image naming in the repo
fd -e md -e txt | xargs rg -l "container|image|ghcr" 2>/dev/null | head -20Repository: orcasound/orcanode
Length of output: 87
🏁 Script executed:
# Check for other docker/metadata-action uses in the repository
rg "docker/metadata-action" .github/workflows/Repository: orcasound/orcanode
Length of output: 186
🏁 Script executed:
# Look at the broader workflow context to see if there are multiple services/images
cat -n .github/workflows/build-container.yml | head -50Repository: orcasound/orcanode
Length of output: 2092
🏁 Script executed:
# Check the README files for any documentation about container image naming
cat README.md | grep -i "container\|image\|ghcr" -A 2 -B 2Repository: orcasound/orcanode
Length of output: 1719
🏁 Script executed:
# Also check the full build-container.yml workflow to understand if there's any reason for the /orcanode suffix
cat -n .github/workflows/build-container.yml | sed -n '130,182p'Repository: orcasound/orcanode
Length of output: 2446
Simplify the image path by removing the redundant /orcanode suffix.
The image path ghcr.io/${{ github.repository }}/orcanode resolves to ghcr.io/orcasound/orcanode/orcanode, which is redundant since only a single container image is built in this workflow. Use:
images: ghcr.io/${{ github.repository }}This produces the simpler and more conventional image path ghcr.io/orcasound/orcanode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-container.yml around lines 155 - 170, Update the
docker/metadata-action step (id: meta) to use a simplified image name under the
images key: change images: ghcr.io/${{ github.repository }}/orcanode to images:
ghcr.io/${{ github.repository }} so the resulting image path becomes
ghcr.io/orcasound/orcanode instead of the redundant
ghcr.io/orcasound/orcanode/orcanode; keep the rest of the metadata-action
configuration (tags block and type/raw latest enable) unchanged.
NODE_TYPE=dev-virt-s3andNODE_TYPE=hls-onlyREADME.mdto match current code: self-contained Dockerfile, correct env var names, node/ compose path, boto3 instead of rsync/s3fs.vsto.gitignorefor those using VS as an IDEREADME.mdsaid to usesyslog+tls:but all current nodes usesyslog:so updated README.md since TLS has not been tested recently. Changing all nodes to use TLS can be tracked in a separate issue..env.examplefile that can be copied and edited/etc/security/limits.conffilename instream.shthat prevented using high prioritySummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores