Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds a new logs CLI command and end-to-end log streaming: runtime StreamLogs API, Docker implementation, container-level orchestration to scan and emit per-line log events, output event/sink support, and integration tests validating streaming and error cases. Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Cmd as logs Command
participant Container as container.Logs
participant Runtime as DockerRuntime
participant Docker as Docker API
participant Sink as Output Sink
User->>Cmd: Run "logs"
Cmd->>Runtime: runtime.NewDockerRuntime()
Cmd->>Container: Logs(ctx, runtime, sink)
Container->>Container: select first container
Container->>Runtime: StreamLogs(ctx, containerID, pipeWriter)
activate Runtime
Runtime->>Docker: GetLogs(Follow, Stdout, Stderr, Tail=all)
Docker-->>Runtime: log stream reader
Runtime->>pipeWriter: stdcopy demux -> writer
deactivate Runtime
Container->>Container: scan pipeReader line-by-line
loop per line
Container->>Sink: EmitContainerLogLine(line)
Sink-->>User: writes line to stdout
end
Container-->>Cmd: return nil or error
Cmd-->>User: exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
internal/output/plain_sink_test.go (1)
143-150: Test looks good, but consider updatingTestPlainSink_UsesFormatterParity.The new test correctly verifies
ContainerLogLineEventemission. However,TestPlainSink_UsesFormatterParity(lines 183-188) tests formatter/sink parity for all event types but doesn't includeContainerLogLineEventin itseventsslice. Based on learnings: "When adding a new event type, update all of: internal/output/events.go (event type + union + emit helper), internal/output/format.go (line formatting fallback), and tests in internal/output/*_test.go for formatter/sink behavior parity."♻️ Add ContainerLogLineEvent to parity test
events := []any{ LogEvent{Message: "hello"}, WarningEvent{Message: "careful"}, ContainerStatusEvent{Phase: "starting", Container: "localstack"}, ProgressEvent{LayerID: "abc", Status: "Downloading", Current: 1, Total: 2}, + ContainerLogLineEvent{Line: "2024-01-01 log line"}, }And add a case in the switch:
case ProgressEvent: Emit(sink, e) + case ContainerLogLineEvent: + Emit(sink, e) default:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/plain_sink_test.go` around lines 143 - 150, Update the parity test TestPlainSink_UsesFormatterParity to include the new ContainerLogLineEvent so formatter/sink parity covers it: add ContainerLogLineEvent to the events slice used in that test and add a corresponding branch in the switch that constructs the expected formatted output for ContainerLogLineEvent (matching NewPlainSink/Emit behavior and the fallback in FormatLine), ensuring the test asserts the same formatted string as the sink emits.internal/runtime/docker.go (1)
163-165: Consider making the error message more generic or parameterized.The error message "emulator is not running" uses LocalStack-specific terminology, but
StreamLogsis a generic runtime method that could stream logs for any container. Consider making this more generic or including the container ID.♻️ Suggested improvement
if errdefs.IsNotFound(err) { - return fmt.Errorf("emulator is not running. Start LocalStack with `lstk`") + return fmt.Errorf("container %s is not running", containerID) }Alternatively, keep the user-friendly message but move it to the
container.Logslayer where the domain context is clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runtime/docker.go` around lines 163 - 165, The error returned in internal/runtime/docker.go inside the errdefs.IsNotFound(err) branch is LocalStack-specific; update the message in the StreamLogs path to be generic and include the container identifier (e.g., use the containerID or containerName variable) so it reads like "container <ID> is not running" (or parameterize the message), and optionally move a user-friendly LocalStack-specific phrasing into the container.Logs layer if domain context is required; change the fmt.Errorf call in the errdefs.IsNotFound(err) block of StreamLogs accordingly.internal/container/logs.go (1)
34-41: Consider closing the pipe reader on early exit to unblock the streaming goroutine.If
scanner.Err()returns an error (line 38-39) before the streaming goroutine completes, the function returns the scanner error but then blocks on<-errCh(line 41). The goroutine may remain blocked onpw.Write()until the context is cancelled. Closingprwould cause the write to fail immediately, unblocking the goroutine.♻️ Proposed fix
scanner := bufio.NewScanner(pr) for scanner.Scan() { output.EmitContainerLogLine(sink, scanner.Text()) } if err := scanner.Err(); err != nil && ctx.Err() == nil { + pr.Close() // unblock the streaming goroutine return err } return <-errCh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/container/logs.go` around lines 34 - 41, When scanner.Scan() stops with an error, close the pipe reader `pr` to unblock the streaming goroutine before returning the scanner error; specifically, in the block that checks `if err := scanner.Err(); err != nil && ctx.Err() == nil { ... }` call `pr.Close()` (or `pr.CloseWithError(err)` if available) and then return the scanner error, so the goroutine blocked on `pw.Write()` wakes and writes to `errCh`; reference `scanner`, `pr`, `pw.Write()`, `errCh`, and `output.EmitContainerLogLine` to locate the surrounding logic.test/integration/logs_test.go (1)
51-52: Fixed sleep may cause flaky tests.The 500ms sleep to wait for the logs connection could be insufficient on slower CI environments or excessive on fast ones. Consider a more robust synchronization approach, such as waiting for initial output or using a ready signal.
That said, for an integration test of this nature, a short sleep is often pragmatic. If flakiness is observed, consider increasing the timeout or adding retry logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/logs_test.go` around lines 51 - 52, Replace the fixed sleep (time.Sleep(500 * time.Millisecond)) used to wait for "lstk" logs with a deterministic wait: poll or block on a ready condition (e.g., an exported channel, a connection-accept event, or a small retry loop checking for initial output) until the logging endpoint is reachable or a short timeout elapses; update the test in logs_test.go to use that readiness check instead of the hardcoded sleep so CI flakiness is avoided while still failing quickly on timeout.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/container/logs.go`:
- Around line 34-41: When scanner.Scan() stops with an error, close the pipe
reader `pr` to unblock the streaming goroutine before returning the scanner
error; specifically, in the block that checks `if err := scanner.Err(); err !=
nil && ctx.Err() == nil { ... }` call `pr.Close()` (or `pr.CloseWithError(err)`
if available) and then return the scanner error, so the goroutine blocked on
`pw.Write()` wakes and writes to `errCh`; reference `scanner`, `pr`,
`pw.Write()`, `errCh`, and `output.EmitContainerLogLine` to locate the
surrounding logic.
In `@internal/output/plain_sink_test.go`:
- Around line 143-150: Update the parity test TestPlainSink_UsesFormatterParity
to include the new ContainerLogLineEvent so formatter/sink parity covers it: add
ContainerLogLineEvent to the events slice used in that test and add a
corresponding branch in the switch that constructs the expected formatted output
for ContainerLogLineEvent (matching NewPlainSink/Emit behavior and the fallback
in FormatLine), ensuring the test asserts the same formatted string as the sink
emits.
In `@internal/runtime/docker.go`:
- Around line 163-165: The error returned in internal/runtime/docker.go inside
the errdefs.IsNotFound(err) branch is LocalStack-specific; update the message in
the StreamLogs path to be generic and include the container identifier (e.g.,
use the containerID or containerName variable) so it reads like "container <ID>
is not running" (or parameterize the message), and optionally move a
user-friendly LocalStack-specific phrasing into the container.Logs layer if
domain context is required; change the fmt.Errorf call in the
errdefs.IsNotFound(err) block of StreamLogs accordingly.
In `@test/integration/logs_test.go`:
- Around line 51-52: Replace the fixed sleep (time.Sleep(500 *
time.Millisecond)) used to wait for "lstk" logs with a deterministic wait: poll
or block on a ready condition (e.g., an exported channel, a connection-accept
event, or a small retry loop checking for initial output) until the logging
endpoint is reachable or a short timeout elapses; update the test in
logs_test.go to use that readiness check instead of the hardcoded sleep so CI
flakiness is avoided while still failing quickly on timeout.
carole-lavillonniere
force-pushed
the
carole/drg-367
branch
from
76a71aa to
8403ab5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/integration/logs_test.go (1)
51-52: Consider a more deterministic synchronization approach.The 500ms sleep to wait for
lstk logsto connect could be flaky on slower CI environments. Consider checking if thelstk logsprocess has produced initial output (e.g., scanning for any line before injecting the marker) or increasing the delay with a comment explaining the rationale.This is a minor concern given the 30s overall timeout provides a safety net.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/logs_test.go` around lines 51 - 52, Replace the fixed time.Sleep(500 * time.Millisecond) wait used to let the "lstk logs" process connect with a deterministic wait that reads the process stdout/stderr and blocks until the first log line appears (or a short timeout), then proceed to inject the marker; update the code around the time.Sleep call and the logic that launches "lstk logs" (search for the time.Sleep invocation and the process launch for "lstk logs") to poll/read the process output stream instead of sleeping to avoid flakes on slow CI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/runtime/docker.go`:
- Around line 162-166: Replace the incorrect containerd errdefs import with the
Docker client errdefs so Docker API errors are correctly detected: update the
import from "github.com/containerd/errdefs" to
"github.com/docker/docker/errdefs" and ensure calls like errdefs.IsNotFound(err)
in runtime/docker.go (the ContainerLogs handling), internal/container/stop.go,
and internal/container/start.go reference the docker/docker errdefs package so
the IsNotFound checks work as intended.
---
Nitpick comments:
In `@test/integration/logs_test.go`:
- Around line 51-52: Replace the fixed time.Sleep(500 * time.Millisecond) wait
used to let the "lstk logs" process connect with a deterministic wait that reads
the process stdout/stderr and blocks until the first log line appears (or a
short timeout), then proceed to inject the marker; update the code around the
time.Sleep call and the logic that launches "lstk logs" (search for the
time.Sleep invocation and the process launch for "lstk logs") to poll/read the
process output stream instead of sleeping to avoid flakes on slow CI.
| if err != nil { | ||
| if errdefs.IsNotFound(err) { | ||
| return fmt.Errorf("emulator is not running. Start LocalStack with `lstk`") | ||
| } | ||
| return fmt.Errorf("failed to stream logs for %s: %w", containerID, err) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the imports in docker.go
echo "=== Checking imports in internal/runtime/docker.go ==="
head -30 internal/runtime/docker.go | cat -n
echo ""
echo "=== Checking all errdefs imports in the codebase ==="
rg "import.*errdefs" --type=go -B2 -A2
echo ""
echo "=== Checking how errdefs is used in the codebase ==="
rg "errdefs\." --type=go -B1 -A1Repository: localstack/lstk
Length of output: 1835
🌐 Web query:
Docker go client github.com/docker/docker ContainerLogs error handling IsNotFound
💡 Result:
When cli.ContainerLogs(...) fails because the container doesn’t exist, the Docker Go client returns an error that should match Docker’s “not found” error-class. Check it with errdefs.IsNotFound(err) (it unwraps the causal chain, so it works even if the error is wrapped). (pkg.go.dev)
rc, err := cli.ContainerLogs(ctx, containerID, opts)
if err != nil {
if errdefs.IsNotFound(err) {
// container does not exist (HTTP 404 / "No such container")
return
}
// other error
return
}
defer rc.Close()Imports:
import (
"github.com/docker/docker/client"
"github.com/docker/docker/errdefs"
)errdefs.IsNotFound is the standard helper for this class of errors. (pkg.go.dev)
Citations:
- 1: https://pkg.go.dev/github.com/docker/docker/errdefs
- 2: https://pkg.go.dev/github.com/docker/docker/errdefs
Fix incorrect errdefs import for Docker client error handling.
The code imports github.com/containerd/errdefs (line 12) but should import github.com/docker/docker/errdefs when working with Docker client errors. The errdefs.IsNotFound() check at line 163 relies on the Docker-specific errdefs package to correctly identify "not found" errors from ContainerLogs(). Using containerd's errdefs may not properly detect Docker API errors, breaking the check for whether the container exists.
Change the import:
- "github.com/containerd/errdefs"
+ "github.com/docker/docker/errdefs"
This issue also affects internal/container/stop.go and internal/container/start.go, which use the same incorrect import pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/runtime/docker.go` around lines 162 - 166, Replace the incorrect
containerd errdefs import with the Docker client errdefs so Docker API errors
are correctly detected: update the import from "github.com/containerd/errdefs"
to "github.com/docker/docker/errdefs" and ensure calls like
errdefs.IsNotFound(err) in runtime/docker.go (the ContainerLogs handling),
internal/container/stop.go, and internal/container/start.go reference the
docker/docker errdefs package so the IsNotFound checks work as intended.
Apparently it's deprecated in docker 🤷 |
2 participants
lstk logsstreams container logs to the terminal until the user cancels with Ctrl+C.