add Dockerfile + compose + workflow -> mattfly/obby-api#3
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Docker containerization and CI workflows, a local Docker Compose development stack, an expanded ChangesContainerized deployment and voice TURN flexibility
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
voice_bridge.go (1)
297-300:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAllow SFU startup when embedded TURN is disabled.
This early return disables all voice features if
VOICE_TURN_SECRETis empty, even whenVOICE_TURN_DISABLE_EMBEDDED=true. That defeats the external-TURN mode.Suggested guard change
- if cfg.TurnAuthSecret == "" { + if cfg.TurnAuthSecret == "" && !cfg.DisableEmbeddedTurn { log.Printf("voice: VOICE_TURN_SECRET unset; voice subsystem disabled") return func() {} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@voice_bridge.go` around lines 297 - 300, The current early return unconditionally disables the voice subsystem when cfg.TurnAuthSecret is empty; change the guard to only disable voice when there is no TURN auth secret and embedded TURN is not explicitly disabled. Specifically, update the conditional that checks cfg.TurnAuthSecret to also check cfg.TurnDisableEmbedded (i.e., only log and return the no-op func when cfg.TurnAuthSecret == "" && !cfg.TurnDisableEmbedded) so external-TURN mode (cfg.TurnDisableEmbedded == true) can start without an embedded secret.voice.go (1)
516-522:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t unconditionally send embedded TURN credentials on join.
When
DisableEmbeddedTurnis enabled, this still mints and sends TURN creds from local config, which can conflict with external TURN provisioning and break client ICE.Suggested direction
- turn := mintTurnCreds(m.cfg, account, 6*time.Hour) + var turn *TurnCreds + if !m.cfg.DisableEmbeddedTurn { + t := mintTurnCreds(m.cfg, account, 6*time.Hour) + turn = &t + } m.send(nick, signalEnvelope{ @@ - TURN: &turn, + TURN: turn,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@voice.go` around lines 516 - 522, The code unconditionally mints and embeds TURN creds before sending a "joined" signal; guard this with the DisableEmbeddedTurn flag so you only call mintTurnCreds and set the TURN field when embedded TURN is allowed. Update the block around mintTurnCreds / signalEnvelope so that if m.cfg.DisableEmbeddedTurn is true you skip mintTurnCreds and send a nil/omitted TURN in the m.send(...) call; if false, continue to mintTurnCreds(account, 6*time.Hour) and include the resulting turn in the signalEnvelope. Ensure you reference the existing mintTurnCreds function, the m.cfg.DisableEmbeddedTurn flag, the signalEnvelope.TURN field, and the m.send(...) invocation when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.env.example:
- Around line 27-31: Add documentation lines to .env.example for the three env
vars referenced by voice.go: VOICE_TURN_DISABLE_EMBEDDED (boolean flag to
disable the embedded TURN server), VOICE_WEBRTC_PORT_MIN (lowest UDP port in the
ephemeral WebRTC port range) and VOICE_WEBRTC_PORT_MAX (highest UDP port in the
WebRTC port range). For each entry include the variable name, a short
description, expected value type (e.g., true/false or numeric port), and the
default/behavior used by voice.go when unset so operators can configure TURN and
port range correctly.
In @.github/workflows/docker-build-push.yml:
- Line 57: The workflow unconditionally creates and pushes a `latest` tag via
the occurrences of `type=raw,value=latest`, which allows PR/branch builds to
overwrite the canonical image; update the workflow so those
`type=raw,value=latest` tag entries (and the related push steps around them) are
only added when running on trusted refs—e.g., wrap or gate the steps that
add/push `latest` with a conditional such as `if: github.ref ==
'refs/heads/main' || startsWith(github.ref, 'refs/tags/')` or move the `latest`
tag logic into a separate job that only runs on main or tag refs, and apply the
same conditional change to the other occurrences noted (the similar entries at
lines 65-66) so non-release/PR builds never tag or push `latest`.
- Around line 30-31: Replace floating action tags with immutable commit SHAs for
every third-party action usage in this workflow (e.g., replace uses:
actions/checkout@v4, docker/setup-buildx-action@..., docker/login-action@...,
docker/build-push-action@..., actions/cache@... with the corresponding full
commit SHA refs). Locate each uses: line (the instances for actions/checkout,
docker/setup-buildx-action, docker/login-action, docker/build-push-action, and
actions/cache) and update the tag to the specific commit digest (the full commit
SHA from the action repo) so the workflow is pinned to an immutable release.
In @.github/workflows/docker-obby-api.yml:
- Line 10: Replace the incorrect glob '**.md' with the correct '**/*.md' in the
workflow so markdown changes in nested directories are matched; update every
occurrence (e.g., the entries currently at the positions shown around the top of
the file) to '**/*.md' inside the workflow's paths/ignore list (look for the
literal '**.md' string and change it to '**/*.md').
In `@compose.yaml`:
- Around line 24-25: The compose file only publishes 3478/udp; when
VOICE_WEBRTC_PORT_MIN and VOICE_WEBRTC_PORT_MAX are configured you must also
publish that UDP port range to allow external ICE to reach SFU candidates—update
the service ports stanza to add a mapping for the range using the
VOICE_WEBRTC_PORT_MIN/VOICE_WEBRTC_PORT_MAX env vars (alongside TURN_PORT),
ensuring the format publishes host-range:container-range/udp (or individual
ports if range unsupported) so the TURN/WebRTC UDP relay and SFU (references:
TURN_PORT, VOICE_WEBRTC_PORT_MIN, VOICE_WEBRTC_PORT_MAX) are reachable from
outside.
---
Outside diff comments:
In `@voice_bridge.go`:
- Around line 297-300: The current early return unconditionally disables the
voice subsystem when cfg.TurnAuthSecret is empty; change the guard to only
disable voice when there is no TURN auth secret and embedded TURN is not
explicitly disabled. Specifically, update the conditional that checks
cfg.TurnAuthSecret to also check cfg.TurnDisableEmbedded (i.e., only log and
return the no-op func when cfg.TurnAuthSecret == "" && !cfg.TurnDisableEmbedded)
so external-TURN mode (cfg.TurnDisableEmbedded == true) can start without an
embedded secret.
In `@voice.go`:
- Around line 516-522: The code unconditionally mints and embeds TURN creds
before sending a "joined" signal; guard this with the DisableEmbeddedTurn flag
so you only call mintTurnCreds and set the TURN field when embedded TURN is
allowed. Update the block around mintTurnCreds / signalEnvelope so that if
m.cfg.DisableEmbeddedTurn is true you skip mintTurnCreds and send a nil/omitted
TURN in the m.send(...) call; if false, continue to mintTurnCreds(account,
6*time.Hour) and include the resulting turn in the signalEnvelope. Ensure you
reference the existing mintTurnCreds function, the m.cfg.DisableEmbeddedTurn
flag, the signalEnvelope.TURN field, and the m.send(...) invocation when making
the change.
🪄 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: 5f5d3f2b-4466-4628-ac58-e110edf777f7
📒 Files selected for processing (9)
.dockerignore.env.example.github/workflows/docker-build-push.yml.github/workflows/docker-obby-api.ymlDockerfilebackendcompose.yamlvoice.govoice_bridge.go
| # --- voice / TURN --- | ||
| VOICE_PUBLIC_IP= # public IP advertised in ICE candidates | ||
| TURN_PORT=3478 # UDP, open in host firewall | ||
| VOICE_TURN_REALM=obby | ||
| VOICE_MAX_ROOM=25 |
There was a problem hiding this comment.
Document the new voice env vars consumed by voice.go.
VOICE_TURN_DISABLE_EMBEDDED, VOICE_WEBRTC_PORT_MIN, and VOICE_WEBRTC_PORT_MAX are read by runtime code but not surfaced in .env.example, which makes this feature hard to configure correctly in deployments.
Suggested additions
# --- voice / TURN ---
VOICE_PUBLIC_IP= # public IP advertised in ICE candidates
TURN_PORT=3478 # UDP, open in host firewall
VOICE_TURN_REALM=obby
VOICE_MAX_ROOM=25
+VOICE_TURN_DISABLE_EMBEDDED=false # true when using external TURN infra
+VOICE_WEBRTC_PORT_MIN=40000 # UDP ICE host candidate range (server-side)
+VOICE_WEBRTC_PORT_MAX=40100📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # --- voice / TURN --- | |
| VOICE_PUBLIC_IP= # public IP advertised in ICE candidates | |
| TURN_PORT=3478 # UDP, open in host firewall | |
| VOICE_TURN_REALM=obby | |
| VOICE_MAX_ROOM=25 | |
| # --- voice / TURN --- | |
| VOICE_PUBLIC_IP= # public IP advertised in ICE candidates | |
| TURN_PORT=3478 # UDP, open in host firewall | |
| VOICE_TURN_REALM=obby | |
| VOICE_MAX_ROOM=25 | |
| VOICE_TURN_DISABLE_EMBEDDED=false # true when using external TURN infra | |
| VOICE_WEBRTC_PORT_MIN=40000 # UDP ICE host candidate range (server-side) | |
| VOICE_WEBRTC_PORT_MAX=40100 |
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 28-28: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 28-28: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 29-29: [UnorderedKey] The TURN_PORT key should go before the VOICE_PUBLIC_IP key
(UnorderedKey)
[warning] 29-29: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 31-31: [UnorderedKey] The VOICE_MAX_ROOM key should go before the VOICE_PUBLIC_IP key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.env.example around lines 27 - 31, Add documentation lines to .env.example
for the three env vars referenced by voice.go: VOICE_TURN_DISABLE_EMBEDDED
(boolean flag to disable the embedded TURN server), VOICE_WEBRTC_PORT_MIN
(lowest UDP port in the ephemeral WebRTC port range) and VOICE_WEBRTC_PORT_MAX
(highest UDP port in the WebRTC port range). For each entry include the variable
name, a short description, expected value type (e.g., true/false or numeric
port), and the default/behavior used by voice.go when unset so operators can
configure TURN and port range correctly.
| - name: Checkout code | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
Pin third-party GitHub Actions to commit SHAs.
Using floating action tags here (@v*) leaves the pipeline open to upstream tag drift/supply-chain compromise. Pin each action to an immutable commit digest.
Also applies to: 34-34, 37-37, 40-40, 47-47, 60-60
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 30-31: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 31-31: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/docker-build-push.yml around lines 30 - 31, Replace
floating action tags with immutable commit SHAs for every third-party action
usage in this workflow (e.g., replace uses: actions/checkout@v4,
docker/setup-buildx-action@..., docker/login-action@...,
docker/build-push-action@..., actions/cache@... with the corresponding full
commit SHA refs). Locate each uses: line (the instances for actions/checkout,
docker/setup-buildx-action, docker/login-action, docker/build-push-action, and
actions/cache) and update the tag to the specific commit digest (the full commit
SHA from the action repo) so the workflow is pinned to an immutable release.
| tags: | ||
| - 'v*' | ||
| paths-ignore: | ||
| - '**.md' |
There was a problem hiding this comment.
Fix markdown ignore glob to match nested files.
**.md is too loose/ambiguous; use **/*.md so docs-only changes reliably skip the workflow.
Suggested patch
- - '**.md'
+ - '**/*.md'
@@
- - '**.md'
+ - '**/*.md'Also applies to: 16-16
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/docker-obby-api.yml at line 10, Replace the incorrect glob
'**.md' with the correct '**/*.md' in the workflow so markdown changes in nested
directories are matched; update every occurrence (e.g., the entries currently at
the positions shown around the top of the file) to '**/*.md' inside the
workflow's paths/ignore list (look for the literal '**.md' string and change it
to '**/*.md').
| ports: | ||
| - "${TURN_PORT:-3478}:3478/udp" |
There was a problem hiding this comment.
Publish the server WebRTC UDP range when using NAT1:1 host candidates.
Only 3478/udp is exposed now. If VOICE_WEBRTC_PORT_MIN/MAX is configured, those UDP ports must also be published or external ICE will fail to connect to SFU candidates.
Suggested patch
ports:
- "${TURN_PORT:-3478}:3478/udp"
+ - "${VOICE_WEBRTC_PORT_MIN:-40000}-${VOICE_WEBRTC_PORT_MAX:-40100}:${VOICE_WEBRTC_PORT_MIN:-40000}-${VOICE_WEBRTC_PORT_MAX:-40100}/udp"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ports: | |
| - "${TURN_PORT:-3478}:3478/udp" | |
| ports: | |
| - "${TURN_PORT:-3478}:3478/udp" | |
| - "${VOICE_WEBRTC_PORT_MIN:-40000}-${VOICE_WEBRTC_PORT_MAX:-40100}:${VOICE_WEBRTC_PORT_MIN:-40000}-${VOICE_WEBRTC_PORT_MAX:-40100}/udp" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@compose.yaml` around lines 24 - 25, The compose file only publishes 3478/udp;
when VOICE_WEBRTC_PORT_MIN and VOICE_WEBRTC_PORT_MAX are configured you must
also publish that UDP port range to allow external ICE to reach SFU
candidates—update the service ports stanza to add a mapping for the range using
the VOICE_WEBRTC_PORT_MIN/VOICE_WEBRTC_PORT_MAX env vars (alongside TURN_PORT),
ensuring the format publishes host-range:container-range/udp (or individual
ports if range unsupported) so the TURN/WebRTC UDP relay and SFU (references:
TURN_PORT, VOICE_WEBRTC_PORT_MIN, VOICE_WEBRTC_PORT_MAX) are reachable from
outside.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.env.example (1)
38-39: ⚡ Quick winPin default image references instead of
latestto avoid non-deterministic deploys.Line 38 and Line 39 use
:latest, which makes environments drift over time and can introduce surprise breakage. Prefer a fixed version tag (or digest) in examples, and let operators opt intolatestexplicitly.Proposed update
-# OBBYIRCD_IMAGE=obbyirc/obbyircd:latest -# OBBY_WEB_IMAGE=obbyirc/obby:latest +# OBBYIRCD_IMAGE=obbyirc/obbyircd:vX.Y.Z +# OBBY_WEB_IMAGE=obbyirc/obby:vX.Y.Z🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.env.example around lines 38 - 39, The example env lines use the non-deterministic :latest tag for OBBYIRCD_IMAGE and OBBY_WEB_IMAGE; change those values to pinned version tags or digests (e.g., replace OBBYIRCD_IMAGE=obbyirc/obbyircd:latest and OBBY_WEB_IMAGE=obbyirc/obby:latest with explicit version tags or image digests) so the example shows deterministic references while leaving it obvious operators can opt into :latest if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.env.example:
- Around line 38-39: The example env lines use the non-deterministic :latest tag
for OBBYIRCD_IMAGE and OBBY_WEB_IMAGE; change those values to pinned version
tags or digests (e.g., replace OBBYIRCD_IMAGE=obbyirc/obbyircd:latest and
OBBY_WEB_IMAGE=obbyirc/obby:latest with explicit version tags or image digests)
so the example shows deterministic references while leaving it obvious operators
can opt into :latest if desired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2444d53-4c76-4d93-93e0-2a78040e6eca
📒 Files selected for processing (3)
.env.exampleDockerfilecompose.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- Dockerfile
- compose.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
voice.go (1)
105-114: 💤 Low valueConsider logging a warning when port range configuration is incomplete or invalid.
If an operator sets only one of
VOICE_WEBRTC_PORT_MINorVOICE_WEBRTC_PORT_MAX, or if Min > Max, the configuration is silently ignored innewWebrtcAPI(line 448). This can be confusing during deployment troubleshooting.💡 Proposed fix to warn on invalid config
if v := os.Getenv("VOICE_WEBRTC_PORT_MIN"); v != "" { if p, err := strconv.Atoi(v); err == nil && p > 0 && p < 65536 { cfg.WebRTCPortMin = uint16(p) } } if v := os.Getenv("VOICE_WEBRTC_PORT_MAX"); v != "" { if p, err := strconv.Atoi(v); err == nil && p > 0 && p < 65536 { cfg.WebRTCPortMax = uint16(p) } } + // Warn if port range is partially configured or invalid + if (cfg.WebRTCPortMin > 0) != (cfg.WebRTCPortMax > 0) { + log.Printf("voice: VOICE_WEBRTC_PORT_MIN and VOICE_WEBRTC_PORT_MAX must both be set; ignoring partial config") + } else if cfg.WebRTCPortMin > 0 && cfg.WebRTCPortMax < cfg.WebRTCPortMin { + log.Printf("voice: VOICE_WEBRTC_PORT_MIN (%d) > VOICE_WEBRTC_PORT_MAX (%d); ignoring invalid range", + cfg.WebRTCPortMin, cfg.WebRTCPortMax) + } return cfg🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@voice.go` around lines 105 - 114, The VOICE_WEBRTC_PORT_MIN/VOICE_WEBRTC_PORT_MAX parsing silently ignores incomplete or invalid ranges; update the parsing in the block that sets cfg.WebRTCPortMin and cfg.WebRTCPortMax to validate both env vars are present and parsed, check 0 < min <= max < 65536, and if only one var is set or min>max or parsing fails emit a warning via the existing logger (use the same logger used elsewhere in this file) describing the invalid/incomplete port-range and the values seen, while leaving cfg.WebRTCPortMin/Max at their defaults so behavior of newWebrtcAPI remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@voice.go`:
- Around line 105-114: The VOICE_WEBRTC_PORT_MIN/VOICE_WEBRTC_PORT_MAX parsing
silently ignores incomplete or invalid ranges; update the parsing in the block
that sets cfg.WebRTCPortMin and cfg.WebRTCPortMax to validate both env vars are
present and parsed, check 0 < min <= max < 65536, and if only one var is set or
min>max or parsing fails emit a warning via the existing logger (use the same
logger used elsewhere in this file) describing the invalid/incomplete port-range
and the values seen, while leaving cfg.WebRTCPortMin/Max at their defaults so
behavior of newWebrtcAPI remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2ed899e5-eb1f-43db-b746-79e00219f926
📒 Files selected for processing (2)
voice.govoice_bridge.go
🚧 Files skipped from review as they are similar to previous changes (1)
- voice_bridge.go
2 participants
Repo-owned Dockerfile, local-dev compose (builds api, pulls obbyircd + obby web), GH workflow pushes mattfly/obby-api. Env vars in .env.example track the surface used by main.go/voice.go.
Companion repos: https://github.com/ObsidianIRC/obby-stack | https://github.com/ObsidianIRC/obby-helm
Summary by CodeRabbit
New Features
Chores