Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughAdds a persistent Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/non_interactive_test.go (1)
49-50: AssertExecute()succeeds in the binding tests.These assertions can still pass after an execution failure because flag parsing happens before
Execute()returns. Checking the error keeps the tests from masking brokenversionwiring.Suggested change
root.SetArgs([]string{"--non-interactive", "version"}) - _ = root.Execute() + if err := root.Execute(); err != nil { + t.Fatalf("expected no error, got %v", err) + } @@ root.SetArgs([]string{"version"}) - _ = root.Execute() + if err := root.Execute(); err != nil { + t.Fatalf("expected no error, got %v", err) + }Also applies to: 60-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/non_interactive_test.go` around lines 49 - 50, The test currently calls root.SetArgs([...]) and ignores the return from root.Execute(), which can hide execution failures; update the tests in cmd/non_interactive_test.go (both the block at lines around the current SetArgs/Execute and the similar block at the later occurrence) to capture the error returned by root.Execute() and assert it is nil (e.g., err := root.Execute(); require.NoError(t, err) or equivalent), so failures in the version command wiring fail the test instead of being masked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/login.go`:
- Around line 20-21: Split the combined guard in cmd/login.go so the
--non-interactive flag produces its own clear error: if cfg.NonInteractive
return an error telling the user that login cannot run with --non-interactive
and to rerun without that flag; otherwise, if !ui.IsInteractive() return the
existing "login requires an interactive terminal" error. Update the conditional
around the login entrypoint (the block currently checking cfg.NonInteractive and
ui.IsInteractive) to check cfg.NonInteractive first, then check
ui.IsInteractive.
In `@test/integration/non_interactive_test.go`:
- Around line 11-40: The tests (TestNonInteractiveFlagBlocksLogin,
TestNonInteractiveFlagFailsWithoutToken,
TestRootNonInteractiveFlagFailsWithoutToken) currently invoke the CLI without a
PTY so ui.IsInteractive() is already false; change them to run the binary inside
a pseudo-terminal using github.com/creack/pty so the flag is the variable under
test. Update each test to spawn the command with pty.Start(cmd) (or add a new
helper like runLstkWithPty that wraps runLstk behavior), stream output with
io.Copy from the PTY to a buffer to capture stderr/stdout, and if needed write
keystrokes to the PTY; assert on the captured output as before to verify
--non-interactive behavior. Ensure the helper preserves environment
modifications (env.Without/With) and returns exit error, stdout/stderr text
similar to the original runLstk contract.
---
Nitpick comments:
In `@cmd/non_interactive_test.go`:
- Around line 49-50: The test currently calls root.SetArgs([...]) and ignores
the return from root.Execute(), which can hide execution failures; update the
tests in cmd/non_interactive_test.go (both the block at lines around the current
SetArgs/Execute and the similar block at the later occurrence) to capture the
error returned by root.Execute() and assert it is nil (e.g., err :=
root.Execute(); require.NoError(t, err) or equivalent), so failures in the
version command wiring fail the test instead of being masked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 6e689a3e-ba7d-44dc-93de-bccc75c56a68
📒 Files selected for processing (7)
cmd/login.gocmd/logout.gocmd/non_interactive_test.gocmd/root.gocmd/stop.gointernal/env/env.gotest/integration/non_interactive_test.go
silv-io
left a comment
There was a problem hiding this comment.
LGTM!
Only thing that needs fixing IMO is the test not using pty.
a58b2dc to
16be97e
Compare
--non-interactiveas a persistent flag on the root commandlstk login --non-interactivefails because it can't open browserlstk startfails whenLOCALSTACK_AUTH_TOKENis not set