test(docker): close SIGTERM, hooksctl-vs-server, 0o755 coverage gaps from PR #2#4
Conversation
Three coverage gaps surfaced in the PR #2 review: - TestImageGracefulShutdownOnSIGTERM: docker stop sends SIGTERM and we expect the binary to honor signal.NotifyContext rather than getting SIGKILL'd at the grace deadline. Asserts exit=0 and elapsed under the binary's 30s shutdown deadline (now extracted to a constant so the source of truth lives in one place). - TestImageHooksctlAgainstRunningServer: runs `hooksctl token list` inside the same container the server is in, against 127.0.0.1:8080. Token from `hooks init` is captured via a redaction-aware helper and passed to docker exec via -e (never argv, never test logs). - TestImageInitFailsClearlyOn0o755HostDir: documents the README-literal case (mkdir -p ./hooks-data without chmod). Probes for UID translation and skips on Docker Desktop / macOS where the scenario doesn't manifest; on Linux CI, asserts init fails with permission-denied.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThree Docker integration tests are added to verify server shutdown behavior, hooksctl token operations against a running server, and initialization error handling under restrictive file permissions. New helpers extract admin tokens from output, redact secrets, and parse token list results. The data scaffolding logic is refactored to capture the one-time admin token during container initialization. ChangesDocker Integration Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🤖 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 `@dockertest/docker_test.go`:
- Around line 445-489: TestImageInitFailsClearlyOn0o755HostDir currently prints
the raw `out` from the `hooks init` run (variable out), which may contain a
one-time admin token/signup code; replace those failing
t.Fatalf/t.Fatalf/t.Fatalf messages so they never include plaintext secrets. Fix
by introducing a redaction step before any log/Fail output (e.g., call a helper
like redactSecrets(out) or sanitizeOutput(out) that strips/masks known token
patterns such as "admin token", "signup code", long hex/base64 strings, or any
regex you use elsewhere) and use the redacted string in the t.Fatalf/t.Fatalf
messages; alternatively only include a short, non-secret summary (exit status or
length) instead of the raw `out`. Ensure the change is applied to the failure
branches that reference `out` in TestImageInitFailsClearlyOn0o755HostDir.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f02376d5-570e-4a7f-8688-8ed82e01ea46
📒 Files selected for processing (1)
dockertest/docker_test.go
When the 0o755 test's first assertion fires, hooks init unexpectedly succeeded — which means it reached the print site in cmd/hooks/main.go and the token is in the captured output. Drop the %s and route through a redaction-aware Fatal, matching the pattern already used in scaffoldDataDir. The second assertion's branch (init returned non-zero, no "permission denied" found) is reached only when init failed before token issuance, so the diagnostic %s there cannot leak the token. Comment the leak analysis so future readers don't reflexively redact and lose the debug output.
|
1 participant
Summary
Closes the three coverage gaps the PR #2 review explicitly deferred:
TestImageGracefulShutdownOnSIGTERM—docker stopsends SIGTERM; we assert the binary honorssignal.NotifyContext(exit 0, elapsed under the 30s shutdown deadline) rather than getting SIGKILL'd. The 30s value is now ashutdownDeadlineconstant so the doc, thedocker stop -targ, and the elapsed-time guard share a single source of truth.TestImageHooksctlAgainstRunningServer— runshooksctl token listinside the same container the server lives in. Captures the admin token fromhooks initstdout viaextractAdminToken(HasPrefix-anchored so a futureinitreformat can't leak it past the helper), passes it via-e HOOKS_TOKEN=(never argv), andredacts any echoed output before it reachest.Fatalf. Assertion uses a whitespace-anchored field match so a future header rename can't false-positive.TestImageInitFailsClearlyOn0o755HostDir— documents the README-literal scenario (mkdir -p ./hooks-datawith default umask 022). Probes for daemon-side UID translation and skips on Docker Desktop / macOS where the scenario doesn't manifest; on Linux CI, assertshooks initfails withpermission deniedrather than silently mis-scaffolding.scaffoldDataDirnow delegates to a newscaffoldDataDirCapturingToken(single body, no behavior change to existing callers).Test plan
make lint— cleango tool golangci-lint run --build-tags=docker ./dockertest/...— cleanmake test— non-docker suite greengo test -tags=docker -count=1 ./dockertest/...(full suite) —okin ~11s-v -run: SIGTERM PASS, hooksctl PASS, 0o755 SKIP (Docker Desktop UID translation, with the documented skip rationale)Review notes (non-blocking, surfaced for the reviewer)
exit==0 && elapsed < 30s. A future regression where the binary panics-and-recovers-to-0 within the window would still pass. Strengthening would mean adding a "shutting down" log line incmd/hooks/main.goand grepping for it — out of scope here.TestImageRestartPreservesStateby stopping via SIGTERM rather thandocker rm -fin a follow-up.runServerContainer(t, dir, name, opts)helper that would dedupe the six near-identicaldocker run -d ...boot blocks (3 pre-existing + 2 new in this PR + 1 implicit) is intentionally not extracted — it touches code from the PR Add Docker support and Render Blueprint #2 surface and is scope creep for this follow-up.Summary by CodeRabbit