Open
Conversation
📝 WalkthroughWalkthroughStandardizes user-facing messages and status output to consistently reference "LocalStack" instead of dynamic container names across start/stop flows and plain text status formatting; tests updated to match the new messages. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 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)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/container/stop.go (1)
12-27: 🛠️ Refactor suggestion | 🟠 MajorRoute stop progress through
output.Sink, notonProgresscallbacks.This path still emits user-visible progress via raw callback strings, which breaks parity with the typed output event pipeline used in
internal/workflows.Proposed direction
package container import ( "context" "fmt" "github.com/containerd/errdefs" "github.com/localstack/lstk/internal/config" + "github.com/localstack/lstk/internal/output" "github.com/localstack/lstk/internal/runtime" ) -func Stop(ctx context.Context, rt runtime.Runtime, onProgress func(string)) error { +func Stop(ctx context.Context, rt runtime.Runtime, sink output.Sink) error { cfg, err := config.Get() if err != nil { return fmt.Errorf("failed to get config: %w", err) } for _, c := range cfg.Containers { - onProgress("Stopping LocalStack...") + output.EmitInfo(sink, "Stopping LocalStack...") if err := rt.Stop(ctx, c.Name()); err != nil { if errdefs.IsNotFound(err) { return fmt.Errorf("LocalStack is not running") } return fmt.Errorf("failed to stop LocalStack: %w", err) } - onProgress("LocalStack stopped") + output.EmitInfo(sink, "LocalStack stopped") }As per coding guidelines
internal/**/*.go: “Any feature/workflow package that produces user-visible progress should accept an output.Sink dependency and emit events through internal/output.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/container/stop.go` around lines 12 - 27, The Stop function currently uses an onProgress callback to emit raw strings; change its signature to accept an output.Sink (instead of onProgress) and emit typed progress events via the internal/output API (e.g., create and write a progress event for "Stopping LocalStack..." and "LocalStack stopped") when iterating cfg.Containers and calling rt.Stop(ctx, c.Name()). Update all references to Stop to pass an output.Sink, remove onProgress usage, and ensure error branches still return the same errors while emitting any final/failure events via the sink.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/container/stop.go`:
- Around line 12-27: The Stop function currently uses an onProgress callback to
emit raw strings; change its signature to accept an output.Sink (instead of
onProgress) and emit typed progress events via the internal/output API (e.g.,
create and write a progress event for "Stopping LocalStack..." and "LocalStack
stopped") when iterating cfg.Containers and calling rt.Stop(ctx, c.Name()).
Update all references to Stop to pass an output.Sink, remove onProgress usage,
and ensure error branches still return the same errors while emitting any
final/failure events via the sink.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/container/start.gointernal/container/stop.gointernal/output/plain_format.gointernal/output/plain_format_test.gointernal/output/plain_sink_test.go
gtsiolis
force-pushed
the
george/replace-container-name
branch
from
719deba to
6eb594e
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/output/plain_format_test.go (1)
51-60: Add table cases for all newly customized status branches.The updated expectations are correct, but only two of the modified
formatStatusLinebranches are asserted here. Please add cases forstarting,waiting,ready(without detail), and default fallback paths to lock in the full behavior change.Suggested test additions
{ name: "status pulling", event: ContainerStatusEvent{Phase: "pulling", Container: "localstack/localstack-pro:latest"}, want: "Preparing LocalStack...", wantOK: true, }, + { + name: "status starting", + event: ContainerStatusEvent{Phase: "starting", Container: "localstack-aws"}, + want: "Starting LocalStack...", + wantOK: true, + }, + { + name: "status waiting", + event: ContainerStatusEvent{Phase: "waiting", Container: "localstack-aws"}, + want: "Waiting for LocalStack to be ready...", + wantOK: true, + }, { name: "status ready with detail", event: ContainerStatusEvent{Phase: "ready", Container: "localstack-aws", Detail: "abc123"}, want: "LocalStack ready (abc123)", wantOK: true, }, + { + name: "status ready without detail", + event: ContainerStatusEvent{Phase: "ready", Container: "localstack-aws"}, + want: "LocalStack ready", + wantOK: true, + }, + { + name: "status default with detail", + event: ContainerStatusEvent{Phase: "restarting", Container: "localstack-aws", Detail: "attempt 1"}, + want: "LocalStack: restarting (attempt 1)", + wantOK: true, + }, + { + name: "status default without detail", + event: ContainerStatusEvent{Phase: "restarting", Container: "localstack-aws"}, + want: "LocalStack: restarting", + wantOK: true, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/plain_format_test.go` around lines 51 - 60, The tests only cover two branches of formatStatusLine; add table-driven cases asserting the other new branches by adding entries to the test cases slice that construct ContainerStatusEvent values for Phase "starting", "waiting", "ready" with no Detail, and a generic unknown phase to exercise the default fallback, and set the expected want strings and wantOK booleans to match the new formatStatusLine behavior so all branches are asserted in the same Test function that iterates over the cases.
🤖 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/output/plain_format_test.go`:
- Around line 51-60: The tests only cover two branches of formatStatusLine; add
table-driven cases asserting the other new branches by adding entries to the
test cases slice that construct ContainerStatusEvent values for Phase
"starting", "waiting", "ready" with no Detail, and a generic unknown phase to
exercise the default fallback, and set the expected want strings and wantOK
booleans to match the new formatStatusLine behavior so all branches are asserted
in the same Test function that iterates over the cases.
ℹ️ Review info
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/container/start.gointernal/container/stop.gointernal/output/plain_format.gointernal/output/plain_format_test.gointernal/output/plain_sink_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/output/plain_sink_test.go
- internal/container/start.go
- internal/container/stop.go
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.
See DES-152 for more context.