Skip to content

Migrate stop command to output event system #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gtsiolis wants to merge 1 commit into
base: main
Choose a base branch
from
+70 −10
Open

Migrate stop command to output event system #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions cmd/stop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
"os"

"github.com/localstack/lstk/internal/container"
"github.com/localstack/lstk/internal/output"
"github.com/localstack/lstk/internal/runtime"
"github.com/localstack/lstk/internal/ui"
"github.com/spf13/cobra"
)

Expand All @@ -22,14 +24,22 @@
os.Exit(1)
}

onProgress := func(msg string) {
fmt.Println(msg)
if ui.IsInteractive() {
if err := ui.RunStop(cmd.Context(), rt); err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}
return
}

if err := container.Stop(cmd.Context(), rt, onProgress); err != nil {
if err := container.Stop(cmd.Context(), rt, output.NewPlainSink(os.Stdout)); err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}
},
}
}

func init() {
rootCmd.AddCommand(newStopCmd())

Check failure on line 44 in cmd/stop.go

View workflow job for this annotation

GitHub Actions / Integration Tests (windows-latest) 

undefined: rootCmd

Check failure on line 44 in cmd/stop.go

View workflow job for this annotation

GitHub Actions / Integration Tests (macos-latest) 

undefined: rootCmd

Check failure on line 44 in cmd/stop.go

View workflow job for this annotation

GitHub Actions / Lint 

undefined: rootCmd) (typecheck)

Check failure on line 44 in cmd/stop.go

View workflow job for this annotation

GitHub Actions / Integration Tests (ubuntu-latest) 

undefined: rootCmd

Check failure on line 44 in cmd/stop.go

View workflow job for this annotation

GitHub Actions / Unit Tests 

undefined: rootCmd
}
Comment on lines +43 to +45
Copy link

@coderabbitai coderabbitai bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Build blocker: rootCmd is undefined in this package.

Line 44 currently fails compilation (undefined: rootCmd) per CI. Register this command through the actual root-command symbol/mechanism used in cmd (or restore the prior registration path).

🧰 Tools
🪛 GitHub Actions: LSTK CI

[error] 44-44: go build -o bin/lstk.exe . failed: undefined: rootCmd (undefined: rootCmd) in cmd/stop.go:44

🪛 GitHub Check: Integration Tests (macos-latest)

[failure] 44-44:
undefined: rootCmd

🪛 GitHub Check: Integration Tests (ubuntu-latest)

[failure] 44-44:
undefined: rootCmd

🪛 GitHub Check: Integration Tests (windows-latest)

[failure] 44-44:
undefined: rootCmd

🪛 GitHub Check: Lint

[failure] 44-44:
undefined: rootCmd) (typecheck)

🪛 GitHub Check: Unit Tests

[failure] 44-44:
undefined: rootCmd

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@cmd/stop.go` around lines 43 - 45, The init() function in cmd/stop.go
attempts to call rootCmd.AddCommand(newStopCmd()) but rootCmd is undefined in
this package; replace the incorrect registration by adding the stop command to
the actual root command symbol used in this module (or restore the previous
registration path). Locate the package's real root command (e.g., a symbol named
RootCmd, GetRootCmd(), or the central command initializer) and change init() to
call that symbol's AddCommand with newStopCmd(), or alternatively export
newStopCmd() and register it where the real root is constructed; ensure init()
references the correct identifier instead of rootCmd so the package compiles.

17 changes: 10 additions & 7 deletions internal/container/stop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,30 @@ 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 {
name := c.Name()
onProgress(fmt.Sprintf("Stopping %s...", name))
running, err := rt.IsRunning(ctx, name)
if err != nil || !running {
return fmt.Errorf("%s is not running", name)
}
Comment on lines +20 to +23
Copy link

@coderabbitai coderabbitai bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Preserve IsRunning infrastructure errors instead of reporting “not running”.

At Line 21, err != nil || !running collapses two different outcomes and drops the original error. Runtime failures (daemon unavailable, permission issues, etc.) will be misreported as “not running”.

💡 Suggested fix 
 		running, err := rt.IsRunning(ctx, name)
-		if err != nil || !running {
+		if err != nil {
+			return fmt.Errorf("failed to check %s status: %w", name, err)
+		}
+		if !running {
 			return fmt.Errorf("%s is not running", name)
 		}
📝 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
running, err := rt.IsRunning(ctx, name)
if err != nil || !running {
return fmt.Errorf("%s is not running", name)
}
running, err := rt.IsRunning(ctx, name)
if err != nil {
return fmt.Errorf("failed to check %s status: %w", name, err)
}
if !running {
return fmt.Errorf("%s is not running", name)
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@internal/container/stop.go` around lines 20 - 23, The current check `err !=
nil || !running` in the IsRunning call masks real runtime errors by always
returning "%s is not running"; change it to handle the two outcomes separately:
if `err != nil` return the underlying error (wrapped for context, e.g.
fmt.Errorf("checking %s running: %w", name, err)), and only if `err == nil &&
!running` return fmt.Errorf("%s is not running", name). Locate the call to
`rt.IsRunning(ctx, name)` in stop.go and update the conditional to preserve and
wrap the original error instead of collapsing both cases.

output.EmitSpinnerStart(sink, fmt.Sprintf("Stopping %s", name))
if err := rt.Stop(ctx, name); err != nil {
if errdefs.IsNotFound(err) {
return fmt.Errorf("%s is not running", name)
}
output.EmitSpinnerStop(sink)
return fmt.Errorf("failed to stop %s: %w", name, err)
}
onProgress(fmt.Sprintf("%s stopped", name))
output.EmitSpinnerStop(sink)
output.EmitSuccess(sink, fmt.Sprintf("%s stopped", name))
}

return nil
Expand Down
47 changes: 47 additions & 0 deletions internal/ui/run_stop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package ui

import (
"context"
"errors"
"os"

tea "github.com/charmbracelet/bubbletea"
"github.com/localstack/lstk/internal/container"
"github.com/localstack/lstk/internal/output"
"github.com/localstack/lstk/internal/runtime"
)

func RunStop(parentCtx context.Context, rt runtime.Runtime) error {
_, cancel := context.WithCancel(parentCtx)
defer cancel()
Comment on lines +15 to +16
Copy link

@coderabbitai coderabbitai bot Mar 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Cancellation context is created but not used by the stop workflow.

Line 15 creates a derived cancelable context, but Line 23 still passes parentCtx to container.Stop. That means UI-triggered cancel won’t reliably stop the background operation, and Line 41 can block waiting for completion.

💡 Suggested fix 
-func RunStop(parentCtx context.Context, rt runtime.Runtime) error {
-	_, cancel := context.WithCancel(parentCtx)
+func RunStop(parentCtx context.Context, rt runtime.Runtime) error {
+	runCtx, cancel := context.WithCancel(parentCtx)
 	defer cancel()
@@
 	go func() {
-		err := container.Stop(parentCtx, rt, output.NewTUISink(programSender{p: p}))
+		err := container.Stop(runCtx, rt, output.NewTUISink(programSender{p: p}))
 		runErrCh <- err

Also applies to: 23-23, 41-44

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@internal/ui/run_stop.go` around lines 15 - 16, The derived cancelable context
created by context.WithCancel(parentCtx) is never used; replace uses of
parentCtx with the derived ctx (the variable returned alongside cancel) so
UI-triggered cancel will propagate to container.Stop and any background waiter;
specifically, pass the derived ctx into container.Stop (instead of parentCtx)
and into the goroutine or wait logic that waits on the done channel (the code
around the container.Stop call and the later wait at the done/wait block), keep
defer cancel() to ensure cleanup.


app := NewApp("", "", "", cancel)
p := tea.NewProgram(app, tea.WithInput(os.Stdin), tea.WithOutput(os.Stdout))
runErrCh := make(chan error, 1)

go func() {
err := container.Stop(parentCtx, rt, output.NewTUISink(programSender{p: p}))
runErrCh <- err
if err != nil && !errors.Is(err, context.Canceled) {
p.Send(runErrMsg{err: err})
return
}
p.Send(runDoneMsg{})
}()

model, err := p.Run()
if err != nil {
return err
}

if app, ok := model.(App); ok && app.Err() != nil {
return app.Err()
}

runErr := <-runErrCh
if runErr != nil && !errors.Is(runErr, context.Canceled) {
return runErr
}

return nil
}
Loading