Conversation
📝 WalkthroughWalkthroughStandardizes user-facing messages to reference "LocalStack" instead of dynamic container names across start/stop flows and plain-text status formatting; tests updated to match the new strings. (≤50 words) 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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
| containerID, err := rt.Start(ctx, c) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to start %s: %w", c.Name, err) | ||
| return fmt.Errorf("failed to start LocalStack: %w", err) |
There was a problem hiding this comment.
nit: This code loops over all containers specified in the config. I know that we only support one container for launch, but the moment we want to support more the output will be confusing.
If someone adds a Snowflake or Azure container, all status messages will still say "LocalStack".
We could also use ProductName() which would read "localstack-pro" instead of "localstack-aws".
I am fine with ignoring this problem for now, just wanted to make you aware of it 🙂
There was a problem hiding this comment.
We also have the Emulator Display name that we're using in the header. Could do that instead?
There was a problem hiding this comment.
Good insight. ⚾
I'd keep it as is.
- The output here can stay consistent with the platform vision and avoid limiting the CLI in SKU-specific messaging, see relevant discussion for more context.
- The emulation-specific information can still live as metadata or sub-information, regardless if one would start AWS, or SNO, both emulators, or side containers with tools (see DRG-364), etc.
There was a problem hiding this comment.
Jsut refreshed and saw your comment above in #63 (comment) after posting, @silv-io.
We also have the Emulator Display name that we're using in the header. Could do that instead?
Do you mean something like this?
diff --git a/internal/container/start.go b/internal/container/start.go
index 8e55be3..85b587d 100644
--- a/internal/container/start.go
+++ b/internal/container/start.go
@@ -122,12 +122,12 @@ func startContainers(ctx context.Context, rt runtime.Runtime, sink output.Sink,
output.EmitStatus(sink, "starting", c.Name, "")
containerID, err := rt.Start(ctx, c)
if err != nil {
- return fmt.Errorf("failed to start LocalStack: %w", err)
+ return fmt.Errorf("failed to start %s: %w", c.ProductName, err)
}
output.EmitStatus(sink, "waiting", c.Name, "")
healthURL := fmt.Sprintf("http://localhost:%s%s", c.Port, c.HealthPath)
- if err := awaitStartup(ctx, rt, sink, containerID, "LocalStack", healthURL); err != nil {
+ if err := awaitStartup(ctx, rt, sink, containerID, c.ProductName, healthURL); err != nil {
return err
}
@@ -144,7 +144,7 @@ func selectContainersToStart(ctx context.Context, rt runtime.Runtime, sink outpu
return nil, fmt.Errorf("failed to check container status: %w", err)
}
if running {
- output.EmitInfo(sink, "LocalStack is already running")
+ output.EmitInfo(sink, fmt.Sprintf("%s is already running", c.ProductName))
continue
}
if err := ports.CheckAvailable(c.Port); err != nil {
diff --git a/internal/container/stop.go b/internal/container/stop.go
index 0e7af84..11a59dd 100644
--- a/internal/container/stop.go
+++ b/internal/container/stop.go
@@ -16,14 +16,18 @@ func Stop(ctx context.Context, rt runtime.Runtime, onProgress func(string)) erro
}
for _, c := range cfg.Containers {
- onProgress("Stopping LocalStack...")
+ productName, err := c.ProductName()
+ if err != nil {
+ productName = "LocalStack"
+ }
+ onProgress(fmt.Sprintf("Stopping %s...", productName))
if err := rt.Stop(ctx, c.Name()); err != nil {
if errdefs.IsNotFound(err) {
- return fmt.Errorf("LocalStack is not running")
+ return fmt.Errorf("%s is not running", productName)
}
- return fmt.Errorf("failed to stop LocalStack: %w", err)
+ return fmt.Errorf("failed to stop %s: %w", productName, err)
}
- onProgress("LocalStack stopped")
+ onProgress(fmt.Sprintf("%s stopped", productName))
}
return nilThere was a problem hiding this comment.
Coming back to this! Rebased, resolved conflicts and pushed.
@silv-io I've thought about this and think adding the emulator-specific name is something we will have to revert later in the output as LocalStack matures as a platform, to avoid legal nounnce of using the AWS or other emulator names in the output, etc.
Using the display name seems quite verbose and repeating (4 times repeating what's in the header).
BEFORE (CURRENT)
▟████▖ lstk (dev)
▟██▙█▙█▟ LocalStack AWS Emulator
▀▛▀▛▀ localhost.localstack.cloud:4566
> Success: Pulled localstack/localstack-pro:latest
LocalStack: validating license (4.14.1.dev108)
Starting LocalStack...
Waiting for LocalStack to be ready...
LocalStack ready (containerId: eadc8fd4f7e1)AFTER
▟████▖ lstk (dev)
▟██▙█▙█▟ LocalStack AWS Emulator
▀▛▀▛▀ localhost.localstack.cloud:4566
> Success: Pulled localstack/localstack-pro:latest
LocalStack AWS Emulator: validating license (4.14.1.dev108)
Starting LocalStack AWS Emulator...
Waiting for LocalStack AWS Emulator to be ready...
LocalStack AWS Emulator ready (containerId: 49cb569fb0bc)What do you think of going with LocalStack for now? Open to both if you think it's better for now.
There was a problem hiding this comment.
It's fine for now to keep as LocalStack, so let's merge it like this.
However, consider the case of starting multiple emulators at once, how are we going to display that then? This is something we'll have to explore further.
carole-lavillonniere
left a comment
carole-lavillonniere
left a comment
There was a problem hiding this comment.
Added one small concern, but it's good to go for me if you think we can ignore it!
gtsiolis
force-pushed
the
george/replace-container-name
branch
from
6eb594e to
42746a2
Compare
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-12: 🛠️ Refactor suggestion | 🟠 MajorDecouple
Stopfrom UI callbacks and emit typed output events instead.Line 12 keeps a UI callback in the container domain, and Lines 19-26 emit pre-rendered strings. This couples domain logic to presentation and conflicts with the output event model.
As per coding guidelines: "internal/{auth,config,container,runtime}//*.go: Do not pass UI callbacks like onProgress func(...) through domain layers; prefer typed output events" and "{cmd,internal}//*.go: Emit typed events through internal/output (EmitInfo, EmitSuccess, EmitNote, EmitWarning, EmitStatus, EmitProgress, etc.) instead of printing from domain/command handlers".
Also applies to: 19-26
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/container/stop.go` at line 12, The Stop function currently accepts a UI callback (onProgress) and emits pre-rendered strings (lines ~19-26); remove the onProgress parameter from Stop(ctx context.Context, rt runtime.Runtime, onProgress func(string)) and instead emit typed output events via the internal/output package (e.g., output.EmitStatus, output.EmitProgress, EmitInfo/EmitSuccess as appropriate) from within Stop and any helper functions; update callers to stop passing UI callbacks and rely on the output event system, and replace all direct string callbacks/usages in the Stop implementation with the corresponding typed output.EmitX calls so domain logic is decoupled from presentation.
🤖 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`:
- Line 12: The Stop function currently accepts a UI callback (onProgress) and
emits pre-rendered strings (lines ~19-26); remove the onProgress parameter from
Stop(ctx context.Context, rt runtime.Runtime, onProgress func(string)) and
instead emit typed output events via the internal/output package (e.g.,
output.EmitStatus, output.EmitProgress, EmitInfo/EmitSuccess as appropriate)
from within Stop and any helper functions; update callers to stop passing UI
callbacks and rely on the output event system, and replace all direct string
callbacks/usages in the Stop implementation with the corresponding typed
output.EmitX calls so domain logic is decoupled from presentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 484e6858-945c-4fc5-a231-260617f1d716
📒 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 (1)
- internal/container/start.go
gtsiolis
force-pushed
the
george/replace-container-name
branch
from
42746a2 to
32143e6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/output/plain_format.go (1)
35-52: Encode the LocalStack-only assumption in the event contract.
ContainerStatusEventstill carries the real container/image, but this formatter now ignores it for every phase and always rendersLocalStack. That matches this flow, but it also makes the event misleading if any other producer reuses it later. Consider making the event/type naming explicitly LocalStack-specific, or adding a separate display label, so the payload remains factual and the formatter’s substitution is intentional.Based on learnings, Event payloads should be domain facts (phase/status/progress), not pre-rendered UI strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/output/plain_format.go` around lines 35 - 52, The formatter formatStatusLine currently hardcodes "LocalStack" while taking a generic ContainerStatusEvent; update the event contract so the payload stays factual: either introduce a LocalStack-specific type (e.g., LocalStackStatusEvent) and use that in formatStatusLine, or add a display label field (e.g., DisplayName) to ContainerStatusEvent and have formatStatusLine render DisplayName instead of the hardcoded "LocalStack"; update all producers/consumers accordingly (create/emit LocalStackStatusEvent or populate DisplayName) and adjust formatStatusLine to reference the new symbol (LocalStackStatusEvent or ContainerStatusEvent.DisplayName) so the substitution is explicit and intentional.
🤖 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.go`:
- Around line 35-52: The formatter formatStatusLine currently hardcodes
"LocalStack" while taking a generic ContainerStatusEvent; update the event
contract so the payload stays factual: either introduce a LocalStack-specific
type (e.g., LocalStackStatusEvent) and use that in formatStatusLine, or add a
display label field (e.g., DisplayName) to ContainerStatusEvent and have
formatStatusLine render DisplayName instead of the hardcoded "LocalStack";
update all producers/consumers accordingly (create/emit LocalStackStatusEvent or
populate DisplayName) and adjust formatStatusLine to reference the new symbol
(LocalStackStatusEvent or ContainerStatusEvent.DisplayName) so the substitution
is explicit and intentional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 18959a58-2998-43fe-acd8-e54465f40d0d
📒 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 (2)
- internal/container/start.go
- internal/container/stop.go
3 participants
See DES-152 for more context.