Feat/docker#10
Open
matheusfillipe wants to merge 16 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces a comprehensive test infrastructure using Testcontainers and pytest, refactors Docker deployment configuration to support WebSocket/RPC capabilities and custom module compilation, updates account registration module initialization, and includes complete Docker documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker/docker-entrypoint.sh (1)
178-183:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun UnrealIRCd in the foreground instead of hiding it behind
tail.This branch is supposed to honor
./unrealircd -F, but it starts a background daemon and makes PID 1 atailprocess instead. That breaks signal propagation and can leave the container looking alive after the server has already died.Suggested fix
-if [ "$1" = "./unrealircd" ] && [ "$2" = "-F" ]; then - # Start unrealircd and tail the logs to keep container running - su-exec unrealircd ./unrealircd start - exec su-exec unrealircd tail -f logs/ircd.log +if [ "${1:-}" = "./unrealircd" ] && [ "${2:-}" = "-F" ]; then + exec su-exec unrealircd ./unrealircd -F else exec su-exec unrealircd "$@" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/docker-entrypoint.sh` around lines 178 - 183, The current branch in docker-entrypoint.sh that checks for "./unrealircd" and "-F" starts the daemon with "su-exec unrealircd ./unrealircd start" and then tails logs, which hides the real server process and breaks signal propagation; change this branch to exec the UnrealIRCd binary directly in foreground (i.e., use exec su-exec unrealircd ./unrealircd -F or otherwise run the server with its foreground flag) so PID 1 is the real server and signals are forwarded correctly, replacing the separate "start" + "tail" sequence in that if block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compose.yaml`:
- Line 20: The healthcheck currently hardcodes the port in the test array
("test: [\"CMD-SHELL\", \"nc -z localhost 6697\"]"), which will drift if
SSL_PORT is overridden; update the healthcheck to use the SSL_PORT environment
variable (e.g., replace the literal 6697 with ${SSL_PORT}) so the container's
healthcheck reads the same value as SSL_PORT and won't be marked unhealthy when
the port is changed.
In `@docker/docker-entrypoint.sh`:
- Around line 8-10: The current defaulting (export WS_PORT="${WS_PORT:-8080}")
makes WS_PORT always non-empty so WS_CONFIG is generated every start; remove the
hard default and treat WS_PORT (and similarly RPC_PORT/RPC_PASSWORD) as truly
optional by exporting their raw values (no fallback) and update the WS_CONFIG
generation logic to only run when WS_PORT is non-empty (use an explicit -n check
on WS_PORT before creating WS_CONFIG); apply the same change to the other
occurrences around the 41-55 block to ensure the WebSocket listener can be
disabled.
In `@docker/README.md`:
- Around line 116-120: The fenced code block containing the filehosts snippet
(the lines starting with "filehosts {" and the "host
"https://files.example.com";" entry) lacks a language tag; update the opening
fence from ``` to a language-specific tag (for example ```hcl or ```caddyfile)
so the block becomes a fenced code block with a language identifier to satisfy
markdownlint MD040.
- Around line 34-37: The WebSocket quick-start points to ws://localhost:8080
which is unreachable because the Ports/Security section states 8080 is
internal-only; update the quick-start line "WebSocket: `ws://localhost:8080`" to
either (a) show the actual published host port (e.g.
`ws://localhost:<published_port>`) and note that you must publish 8080 in your
docker run or compose, or (b) recommend connecting via a TLS-terminated reverse
proxy using `wss://localhost` if you expose only 443; reference the existing
"WebSocket: `ws://localhost:8080`" string and the "Ports/Security" section when
making the change so readers know why they must publish the port or use a proxy.
In `@obsidianirc.c`:
- Around line 41-46: The config test currently registers accreg_configtest via
MOD_TEST() after calling set_accreg_conf(), but set_accreg_conf() does not reset
the MyConf.got_* duplicate-tracking flags so subsequent config tests can see
stale "got" values; update the initialization path to clear all MyConf.got_*
flags before each config test run—either by adding a routine that explicitly
zeroes MyConf.got_* and calling it at the start of set_accreg_conf() or by
clearing those flags in MOD_TEST() before HookAdd(modinfo->handle,
HOOKTYPE_CONFIGTEST, 0, accreg_configtest) so accreg_configtest always runs with
fresh duplicate-tracking state.
In `@tests/test_server.py`:
- Around line 34-36: CAP LS 302 can be multi-line so reading just the first
matching line (via c.send("CAP LS 302") and line = c.wait_for("CAP", " LS "))
may miss capabilities; change the test to repeatedly call c.wait_for("CAP", " LS
") after c.send("CAP LS 302"), accumulate all returned lines into a single
string or list until the server indicates the end of the CAP LS response (no
continuation), then assert "sasl" (and similarly "draft/account-registration")
is present in the combined response; apply this same accumulation fix to the
other occurrence around lines 42-44.
---
Outside diff comments:
In `@docker/docker-entrypoint.sh`:
- Around line 178-183: The current branch in docker-entrypoint.sh that checks
for "./unrealircd" and "-F" starts the daemon with "su-exec unrealircd
./unrealircd start" and then tails logs, which hides the real server process and
breaks signal propagation; change this branch to exec the UnrealIRCd binary
directly in foreground (i.e., use exec su-exec unrealircd ./unrealircd -F or
otherwise run the server with its foreground flag) so PID 1 is the real server
and signals are forwarded correctly, replacing the separate "start" + "tail"
sequence in that if block.
🪄 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: a9cc92f5-ea17-4f5d-a706-c40766762848
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.env.test.github/workflows/e2e.yml.gitignorecompose.yamldocker/DOCKER_README.mddocker/Dockerfiledocker/README.mddocker/docker-entrypoint.shdocker/unrealircd.conf.templateobsidian.hobsidianirc.cpyproject.tomltests/__init__.pytests/conftest.pytests/helpers.pytests/test_custom_modules.pytests/test_server.py
💤 Files with no reviewable changes (2)
- obsidian.h
- docker/DOCKER_README.md
| timeout: 10s | ||
| retries: 3 | ||
| start_period: 40s | ||
| test: ["CMD-SHELL", "nc -z localhost 6697"] |
There was a problem hiding this comment.
Use SSL_PORT in healthcheck to avoid config drift.
Line 20 hardcodes 6697; if SSL_PORT is overridden through .env, the service can be healthy but marked unhealthy.
Proposed fix
- test: ["CMD-SHELL", "nc -z localhost 6697"]
+ test: ["CMD-SHELL", "nc -z localhost $${SSL_PORT:-6697}"]📝 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.
Suggested change
| test: ["CMD-SHELL", "nc -z localhost 6697"] | |
| test: ["CMD-SHELL", "nc -z localhost $${SSL_PORT:-6697}"] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@compose.yaml` at line 20, The healthcheck currently hardcodes the port in the
test array ("test: [\"CMD-SHELL\", \"nc -z localhost 6697\"]"), which will drift
if SSL_PORT is overridden; update the healthcheck to use the SSL_PORT
environment variable (e.g., replace the literal 6697 with ${SSL_PORT}) so the
container's healthcheck reads the same value as SSL_PORT and won't be marked
unhealthy when the port is changed.
Comment on lines
+8
to
+10
| export WS_PORT="${WS_PORT:-8080}" | ||
| export RPC_PORT="${RPC_PORT:-8600}" | ||
| export RPC_PASSWORD="${RPC_PASSWORD:-}" |
There was a problem hiding this comment.
WS_PORT is not actually optional with the current defaulting.
"${WS_PORT:-8080}" is always non-empty, so WS_CONFIG is generated on every start. That makes the "only if WS_PORT is set" contract impossible to honor and removes any way to disable the WebSocket listener.
Suggested fix
-export WS_PORT="${WS_PORT:-8080}"
+export WS_PORT="${WS_PORT-}"
...
-if [ -n "$WS_PORT" ]; then
+if [ -n "${WS_PORT:-}" ]; thenAlso applies to: 41-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker/docker-entrypoint.sh` around lines 8 - 10, The current defaulting
(export WS_PORT="${WS_PORT:-8080}") makes WS_PORT always non-empty so WS_CONFIG
is generated every start; remove the hard default and treat WS_PORT (and
similarly RPC_PORT/RPC_PASSWORD) as truly optional by exporting their raw values
(no fallback) and update the WS_CONFIG generation logic to only run when WS_PORT
is non-empty (use an explicit -n check on WS_PORT before creating WS_CONFIG);
apply the same change to the other occurrences around the 41-55 block to ensure
the WebSocket listener can be disabled.
Comment on lines
+34
to
+37
| 4. **Connect to your server**: | ||
| - **SSL/TLS IRC**: `ircs://localhost:6697` (uses self-signed cert by default) | ||
| - **WebSocket**: `ws://localhost:8080` (plain; put behind a TLS reverse proxy for production) | ||
| - **Default oper**: username `admin`, password `admin123` |
There was a problem hiding this comment.
Fix the WebSocket quick-start example.
This section tells users to connect to ws://localhost:8080, but the later Ports/Security sections say 8080 is internal-only and not published by default. As written, the first-run instructions point at an unreachable endpoint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker/README.md` around lines 34 - 37, The WebSocket quick-start points to
ws://localhost:8080 which is unreachable because the Ports/Security section
states 8080 is internal-only; update the quick-start line "WebSocket:
`ws://localhost:8080`" to either (a) show the actual published host port (e.g.
`ws://localhost:<published_port>`) and note that you must publish 8080 in your
docker run or compose, or (b) recommend connecting via a TLS-terminated reverse
proxy using `wss://localhost` if you expose only 443; reference the existing
"WebSocket: `ws://localhost:8080`" string and the "Ports/Security" section when
making the change so readers know why they must publish the port or use a proxy.
Comment on lines
+116
to
+120
| ``` | ||
| filehosts { | ||
| host "https://files.example.com"; | ||
| }; | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to this fenced block.
This snippet is missing a fence language, so markdownlint will keep flagging MD040 here.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 116-116: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker/README.md` around lines 116 - 120, The fenced code block containing
the filehosts snippet (the lines starting with "filehosts {" and the "host
"https://files.example.com";" entry) lacks a language tag; update the opening
fence from ``` to a language-specific tag (for example ```hcl or ```caddyfile)
so the block becomes a fenced code block with a language identifier to satisfy
markdownlint MD040.
Comment on lines
+41
to
+46
| MOD_TEST() | ||
| { | ||
| set_accreg_conf(); | ||
| HookAdd(modinfo->handle, HOOKTYPE_CONFIGTEST, 0, accreg_configtest); | ||
| return MOD_SUCCESS; | ||
| } |
There was a problem hiding this comment.
Reset MyConf.got_* tracking before each config test run.
Line 44 enables accreg_configtest, but Line 43 only calls set_accreg_conf() which currently does not clear duplicate-tracking flags. After one config test pass, later config tests can falsely report duplicates for valid directives.
Proposed fix
void set_accreg_conf(void)
{
+ safe_free(MyConf.guest_nick_format);
+ memset(&MyConf, 0, sizeof(MyConf));
+
MyConf.min_name_length = 3;
MyConf.max_name_length = 50;
MyConf.min_password_length = 8;
MyConf.max_password_length = 200;
MyConf.require_email = 1;
MyConf.require_terms_acceptance = 1;
MyConf.allow_username_changes = 1;
MyConf.allow_password_changes = 1;
MyConf.allow_email_changes = 1;
safe_strdup(MyConf.guest_nick_format, "Guest$d$d$d$d");
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@obsidianirc.c` around lines 41 - 46, The config test currently registers
accreg_configtest via MOD_TEST() after calling set_accreg_conf(), but
set_accreg_conf() does not reset the MyConf.got_* duplicate-tracking flags so
subsequent config tests can see stale "got" values; update the initialization
path to clear all MyConf.got_* flags before each config test run—either by
adding a routine that explicitly zeroes MyConf.got_* and calling it at the start
of set_accreg_conf() or by clearing those flags in MOD_TEST() before
HookAdd(modinfo->handle, HOOKTYPE_CONFIGTEST, 0, accreg_configtest) so
accreg_configtest always runs with fresh duplicate-tracking state.
Comment on lines
+34
to
+36
| c.send("CAP LS 302") | ||
| line = c.wait_for("CAP", " LS ") | ||
| assert "sasl" in line |
There was a problem hiding this comment.
Collect the full CAP LS response before asserting capabilities.
CAP LS 302 can span multiple lines. These tests only inspect the first matching CAP LS line, so they will start failing once sasl or draft/account-registration moves to a continuation line.
Also applies to: 42-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_server.py` around lines 34 - 36, CAP LS 302 can be multi-line so
reading just the first matching line (via c.send("CAP LS 302") and line =
c.wait_for("CAP", " LS ")) may miss capabilities; change the test to repeatedly
call c.wait_for("CAP", " LS ") after c.send("CAP LS 302"), accumulate all
returned lines into a single string or list until the server indicates the end
of the CAP LS response (no continuation), then assert "sasl" (and similarly
"draft/account-registration") is present in the combined response; apply this
same accumulation fix to the other occurrence around lines 42-44.
5 tasks
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
1 participant
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 by CodeRabbit
New Features
Tests
Documentation
Chores