Open
Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds interactive AWS profile setup flows and commands: new top-level Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as "CLI\n(config profile / setup aws)"
participant Resolver as "Endpoint\nResolver"
participant UI as "TUI\nRunConfigProfile"
participant Setup as "awsconfig\n(Check/Ensure/Setup)"
participant FS as "Filesystem\n(~/.aws/*)"
User->>CLI: invoke 'setup aws' / 'config profile'
CLI->>Resolver: resolve host (container port, localStackHost)
alt DNS resolution failed
Resolver-->>CLI: note fallback to 127.0.0.1
end
CLI->>UI: RunConfigProfile(ctx, containers, resolvedHost)
UI->>UI: start Bubble Tea program (stdin/stdout)
UI->>Setup: CheckProfileStatus(resolvedHost)
UI->>Setup: run Setup/EnsureProfile
par Prompt/interaction
Setup->>User: prompt "Set up a LocalStack profile for AWS CLI and SDKs in ~/.aws?" [Y/n]
User->>Setup: respond (Y / n) or cancel
UI->>UI: render TUI loop and progress
end
alt user confirms
Setup->>FS: write ~/.aws/config and/or ~/.aws/credentials
Setup-->>UI: success message ("Created ...")
else user declines
Setup-->>UI: emit "Skipped ..." message
else context cancelled
Setup-->>UI: return without completion message
end
UI-->>CLI: return final error or nil
CLI-->>User: exit
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 docstrings
🧪 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.
Actionable comments posted: 2
🤖 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/awsconfig/awsconfig.go`:
- Around line 251-255: The current code in Setup calls needsProfileSetup and
converts errors into warnings (using output.EmitWarning) then returns nil;
change Setup to propagate real errors instead of swallowing them — when
needsProfileSetup (and the subsequent file-write paths around the same block)
returns an error, return that error from Setup; remove the early
EmitWarning/return-nil behavior in Setup and only downgrade to warnings inside
NotifyIfMissing where startup is best-effort; update callers accordingly so they
handle the propagated errors from Setup and retain output.EmitWarning(sink, ...)
only in NotifyIfMissing.
In `@test/integration/awsconfig_test.go`:
- Around line 76-77: The tests currently drop the error returned by cmd.Wait()
after reading from outputCh, so a non-zero subprocess exit won't fail the test;
modify both PTY start test blocks to capture and assert the result of cmd.Wait()
(e.g., err := cmd.Wait(); assert.NoError(t, err) or require.NoError(t, err))
immediately after <-outputCh, referring to the existing cmd variable and
outputCh channel in those tests, and make the same change in the other identical
test branch that also calls cmd.Wait() and <-outputCh.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b33a310b-c477-43f9-8d5a-4769db0eaa91
📒 Files selected for processing (6)
cmd/config.gointernal/awsconfig/awsconfig.gointernal/container/start.gointernal/ui/components/input_prompt_test.gointernal/ui/run_awsconfig.gotest/integration/awsconfig_test.go
anisaoshafi
approved these changes
Mar 20, 2026
Contributor
anisaoshafi
left a comment
anisaoshafi
left a comment
There was a problem hiding this comment.
LGTM, tested and worked well for me 🚀
silv-io
approved these changes
Mar 20, 2026
Member
silv-io
left a comment
silv-io
left a comment
There was a problem hiding this comment.
LGTM feature wise but it will require some cleanup after.
If this doesn't need to go into the release I'd fix that issue before merging, though.
gtsiolis
force-pushed
the
george/des-178
branch
from
a2aed95 to
da25541
Compare
Collaborator
|
@gtsiolis How do we envision I am concerned a "profile" is a very aws-specific thing and I feel that "lstk config profile" does not fit our existing naming scheme because:
What about |
| } | ||
| } | ||
| return nil | ||
| } |
Collaborator
There was a problem hiding this comment.
nit: this method does not really belong in this file in my opinion, it could be moved to the call place and it does not need to be exported.
Collaborator
There was a problem hiding this comment.
Looks like this is still not addressed?
gtsiolis
force-pushed
the
george/des-178
branch
2 times, most recently
from
8cbfcc3 to
11bc372
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/awsconfig/awsconfig.go (1)
284-296:⚠️ Potential issue | 🟡 MinorDon't always claim
~/.aws/configwas created.
status.configNeededandstatus.credsNeededcan differ, but this always emits a config-file creation message.lstk setup awscan therefore report the wrong thing when it only repaired credentials.Possible tweak
- output.EmitSuccess(sink, "Created LocalStack profile in ~/.aws/config") + output.EmitSuccess(sink, "Updated LocalStack AWS profile in ~/.aws.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/awsconfig/awsconfig.go` around lines 284 - 296, The success message always claims "~/.aws/config" was created even when only credentials were updated; update the logic around status.configNeeded and status.credsNeeded (and the calls to writeConfigProfile and writeCredsProfile) so output.EmitSuccess(sink, ...) reflects what actually changed: emit a config-created message only if status.configNeeded and writeConfigProfile succeeded, emit a credentials-created/updated message only if status.credsNeeded and writeCredsProfile succeeded, and if both succeeded emit a combined message; use the same sink and preserve existing warning behavior when writes fail.
♻️ Duplicate comments (2)
test/integration/awsconfig_test.go (1)
76-77:⚠️ Potential issue | 🟡 MinorAssert
cmd.Wait()in both PTY start tests.A non-zero subprocess exit after the prompt/note is rendered still leaves these tests green right now.
Small test fix
- _ = cmd.Wait() + err = cmd.Wait() <-outputCh + require.NoError(t, err)Also applies to: 201-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/awsconfig_test.go` around lines 76 - 77, The test currently discards the subprocess exit status with `_ = cmd.Wait()` allowing a non-zero exit to still pass; change this to assert the wait result (e.g., capture `err := cmd.Wait()` and fail the test if err != nil or use your test assertion helper) so any non-zero subprocess exit fails the test; do this in the PTY start tests that use `cmd.Wait()` and `outputCh` and also update the duplicate occurrence around the other PTY start test (the similar `cmd.Wait()` / `<-outputCh` pair mentioned).internal/awsconfig/awsconfig.go (1)
241-243:⚠️ Potential issue | 🟠 MajorPropagate failures from
Setup.
Setupnow backs the explicit setup command, so returningnilon path/check/write failures makes the command succeed even when nothing was fixed. The warning-only downgrade belongs inEnsureProfile, where startup is best-effort.Suggested direction
func EnsureProfile(ctx context.Context, sink output.Sink, interactive bool, resolvedHost string) error { ... if interactive && !configOK && !credsOK { - return Setup(ctx, sink, resolvedHost) + if err := Setup(ctx, sink, resolvedHost); err != nil { + output.EmitWarning(sink, err.Error()) + } + return nil } ... } func Setup(ctx context.Context, sink output.Sink, resolvedHost string) error { status, err := needsProfileSetup(resolvedHost) if err != nil { - output.EmitWarning(sink, fmt.Sprintf("could not check AWS profile: %v", err)) - return nil + return fmt.Errorf("could not check AWS profile: %w", err) } ... configPath, credsPath, err := awsPaths() if err != nil { - output.EmitWarning(sink, fmt.Sprintf("could not determine AWS config paths: %v", err)) - return nil + return fmt.Errorf("could not determine AWS config paths: %w", err) } ... if status.configNeeded { if err := writeConfigProfile(configPath, resolvedHost); err != nil { - output.EmitWarning(sink, fmt.Sprintf("could not update ~/.aws/config: %v", err)) - return nil + return fmt.Errorf("could not update ~/.aws/config: %w", err) } } if status.credsNeeded { if err := writeCredsProfile(credsPath); err != nil { - output.EmitWarning(sink, fmt.Sprintf("could not update ~/.aws/credentials: %v", err)) - return nil + return fmt.Errorf("could not update ~/.aws/credentials: %w", err) } }Also applies to: 252-265, 285-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/awsconfig/awsconfig.go` around lines 241 - 243, The interactive code paths currently call Setup but swallow its failures; change those branches so any error returned by Setup is propagated up (i.e., return the error from Setup) instead of returning nil/ignoring it; specifically update the branches that call Setup under the conditions interactive && !configOK && !credsOK and the similar blocks around the EnsureProfile logic (the code sections you noted at lines ~241, ~252-265, and ~285-293) so Setup errors are returned to the caller, and keep the downgrade-to-warning behavior confined to EnsureProfile rather than masking Setup failures.
🧹 Nitpick comments (1)
test/integration/awsconfig_test.go (1)
215-216: Add at least one integration path throughsetup aws.Both new interactive tests still exercise
config profile, so the public command introduced incmd/setup.gocan regress without coverage. Switching one of these flows tosetup awswould lock down the new entrypoint.As per coding guidelines, "Prefer integration tests to cover most cases. Use unit tests when integration tests are not practical."
Also applies to: 265-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/awsconfig_test.go` around lines 215 - 216, The integration tests in test/integration/awsconfig_test.go currently only exercise "config profile" (via exec.CommandContext(ctx, binaryPath(), "config", "profile")), so add at least one test flow that invokes the new public entrypoint by running the binary with arguments "setup", "aws" (e.g., exec.CommandContext(ctx, binaryPath(), "setup", "aws")), reusing ctx, baseEnv and binaryPath(), and update the test's simulated stdin/expectations to match the interactive prompts from cmd/setup.go's setup aws handler so the new entrypoint is covered by an integration path.
🤖 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/setup.go`:
- Around line 15-17: Update the CLI help text for the setup command so it
explicitly lists emulator types and notes only aws is implemented: modify the
Cobra command fields (Use, Short, Long) for the setup command (e.g., setupCmd's
Use/Short/Long) to mention "emulator types: aws, snowflake, azure" and clarify
"only aws is currently supported" instead of saying "Azure config"; ensure the
Long description reflects supported types and current implementation status.
In `@internal/awsconfig/awsconfig.go`:
- Around line 187-189: The follow-up note emitted by emitMissingProfileNote
still references the old command; update the message passed to output.EmitNote
in function emitMissingProfileNote to instruct users to run the new entrypoint
"lstk setup aws" (e.g., replace "Run 'lstk config profile'." with "Run 'lstk
setup aws'.") so the note points at the new explicit setup flow; ensure you only
change the string passed to output.EmitNote in emitMissingProfileNote.
In `@test/integration/awsconfig_test.go`:
- Around line 139-140: The negative assertions in awsconfig_test.go are checking
the wrong substring ("Set up a LocalStack profile for AWS CLI and SDKs?") so
they can pass even if the full prompt regresses; update the assert.NotContains
calls inside TestStartPromptsWhenAWSProfileConfigured (and the similar checks at
lines 204-205) to reuse the exact full start prompt string used in
TestStartPromptsWhenAWSProfileMissingEverywhere (or extract that full prompt
into a shared variable) so the NotContains checks validate against the complete
prompt "Set up a LocalStack profile for AWS CLI and SDKs in ~/.aws?".
---
Outside diff comments:
In `@internal/awsconfig/awsconfig.go`:
- Around line 284-296: The success message always claims "~/.aws/config" was
created even when only credentials were updated; update the logic around
status.configNeeded and status.credsNeeded (and the calls to writeConfigProfile
and writeCredsProfile) so output.EmitSuccess(sink, ...) reflects what actually
changed: emit a config-created message only if status.configNeeded and
writeConfigProfile succeeded, emit a credentials-created/updated message only if
status.credsNeeded and writeCredsProfile succeeded, and if both succeeded emit a
combined message; use the same sink and preserve existing warning behavior when
writes fail.
---
Duplicate comments:
In `@internal/awsconfig/awsconfig.go`:
- Around line 241-243: The interactive code paths currently call Setup but
swallow its failures; change those branches so any error returned by Setup is
propagated up (i.e., return the error from Setup) instead of returning
nil/ignoring it; specifically update the branches that call Setup under the
conditions interactive && !configOK && !credsOK and the similar blocks around
the EnsureProfile logic (the code sections you noted at lines ~241, ~252-265,
and ~285-293) so Setup errors are returned to the caller, and keep the
downgrade-to-warning behavior confined to EnsureProfile rather than masking
Setup failures.
In `@test/integration/awsconfig_test.go`:
- Around line 76-77: The test currently discards the subprocess exit status with
`_ = cmd.Wait()` allowing a non-zero exit to still pass; change this to assert
the wait result (e.g., capture `err := cmd.Wait()` and fail the test if err !=
nil or use your test assertion helper) so any non-zero subprocess exit fails the
test; do this in the PTY start tests that use `cmd.Wait()` and `outputCh` and
also update the duplicate occurrence around the other PTY start test (the
similar `cmd.Wait()` / `<-outputCh` pair mentioned).
---
Nitpick comments:
In `@test/integration/awsconfig_test.go`:
- Around line 215-216: The integration tests in
test/integration/awsconfig_test.go currently only exercise "config profile" (via
exec.CommandContext(ctx, binaryPath(), "config", "profile")), so add at least
one test flow that invokes the new public entrypoint by running the binary with
arguments "setup", "aws" (e.g., exec.CommandContext(ctx, binaryPath(), "setup",
"aws")), reusing ctx, baseEnv and binaryPath(), and update the test's simulated
stdin/expectations to match the interactive prompts from cmd/setup.go's setup
aws handler so the new entrypoint is covered by an integration path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7421a921-09ad-436f-9097-56c5b58c28bb
📒 Files selected for processing (10)
CLAUDE.mdcmd/config.gocmd/root.gocmd/setup.gointernal/awsconfig/awsconfig.gointernal/config/containers.gointernal/container/start.gointernal/ui/components/input_prompt_test.gointernal/ui/run_awsconfig.gotest/integration/awsconfig_test.go
✅ Files skipped from review due to trivial changes (3)
- internal/ui/components/input_prompt_test.go
- CLAUDE.md
- internal/container/start.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/ui/run_awsconfig.go
- cmd/config.go
gtsiolis
force-pushed
the
george/des-178
branch
2 times, most recently
from
83235ac to
c968f27
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
test/integration/awsconfig_test.go (2)
76-77:⚠️ Potential issue | 🟡 MinorAssert subprocess exit status in PTY tests.
Line 76 and Line 201 discard
cmd.Wait()errors, so a failing subprocess can still let the test pass.Patch
- _ = cmd.Wait() - <-outputCh + err = cmd.Wait() + <-outputCh + require.NoError(t, err) ... - _ = cmd.Wait() - <-outputCh + err = cmd.Wait() + <-outputCh + require.NoError(t, err)Also applies to: 201-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/awsconfig_test.go` around lines 76 - 77, The test currently discards the subprocess exit error by doing `_ = cmd.Wait()` which can hide failures; update both occurrences where `cmd.Wait()` is ignored (the one paired with `<-outputCh` and the other at the later location) to assert the wait result and fail the test on error—e.g., call `err := cmd.Wait()` and use the test helper (`t.Fatalf`, `t.Fatalff`, or your project’s assertion helper like `require.NoError(t, err)`) to report the error so failing subprocesses cause the test to fail; keep the `<-outputCh` synchronization but ensure you assert the `cmd.Wait()` error afterward.
139-140:⚠️ Potential issue | 🟠 MajorUse one shared full prompt literal across all prompt assertions.
These checks mix prompt variants; the shorter
"Set up ... SDKs?"does not match the current full prompt text and can cause false assertions/timeouts.Patch
+const awsProfilePrompt = "Set up a LocalStack profile for AWS CLI and SDKs in ~/.aws?" ... - assert.NotContains(t, out.String(), "Set up a LocalStack profile for AWS CLI and SDKs?", + assert.NotContains(t, out.String(), awsProfilePrompt, "profile prompt should not appear when profile is already correctly configured") ... - assert.NotContains(t, out.String(), "Set up a LocalStack profile for AWS CLI and SDKs?", + assert.NotContains(t, out.String(), awsProfilePrompt, "profile prompt should not appear for a partial setup") ... - return bytes.Contains(out.Bytes(), []byte("Set up a LocalStack profile for AWS CLI and SDKs?")) + return bytes.Contains(out.Bytes(), []byte(awsProfilePrompt)) ... - return bytes.Contains(out.Bytes(), []byte("Set up a LocalStack profile for AWS CLI and SDKs?")) + return bytes.Contains(out.Bytes(), []byte(awsProfilePrompt))Also applies to: 204-205, 229-232, 279-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/awsconfig_test.go` around lines 139 - 140, Replace the varied short prompt literal with a single shared full prompt constant used by all assertions: define a descriptive variable (e.g., localstackFullPrompt) containing the complete prompt text and update every assert.Contains / assert.NotContains that checks the LocalStack prompt (the assertion currently using "Set up a LocalStack profile for AWS CLI and SDKs?") to reference that variable instead (affecting the assertions around the locations noted, including the existing assertion and the ones at the other mentioned ranges).
🧹 Nitpick comments (1)
test/integration/awsconfig_test.go (1)
215-216: Add coverage for thesetup awsentrypoint.These integration tests currently exercise
config profile; add (or switch one path to)setup awsso the user-facing setup command is regression-protected directly.Also applies to: 265-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/awsconfig_test.go` around lines 215 - 216, The integration test currently invokes the CLI via exec.CommandContext(ctx, binaryPath(), "config", "profile"); update that (and the other occurrence around the later block) to exercise the user-facing setup entrypoint by either replacing or adding a command invocation that calls exec.CommandContext(ctx, binaryPath(), "setup", "aws") so the integration test runs the "setup aws" flow; ensure you preserve cmd.Env = baseEnv and any assertions/cleanup so the new path is covered and assertions reference the same expected outputs for the AWS setup flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/integration/awsconfig_test.go`:
- Line 160: Update the test assertion to match the new guidance text: replace
the expected substring "LocalStack AWS profile is incomplete. Run 'lstk config
profile'." with "LocalStack AWS profile is incomplete. Run 'lstk setup aws'." in
the assert.Contains call that checks stdout (the assertion using
assert.Contains(t, stdout, ...)) so the test matches the emitted message.
---
Duplicate comments:
In `@test/integration/awsconfig_test.go`:
- Around line 76-77: The test currently discards the subprocess exit error by
doing `_ = cmd.Wait()` which can hide failures; update both occurrences where
`cmd.Wait()` is ignored (the one paired with `<-outputCh` and the other at the
later location) to assert the wait result and fail the test on error—e.g., call
`err := cmd.Wait()` and use the test helper (`t.Fatalf`, `t.Fatalff`, or your
project’s assertion helper like `require.NoError(t, err)`) to report the error
so failing subprocesses cause the test to fail; keep the `<-outputCh`
synchronization but ensure you assert the `cmd.Wait()` error afterward.
- Around line 139-140: Replace the varied short prompt literal with a single
shared full prompt constant used by all assertions: define a descriptive
variable (e.g., localstackFullPrompt) containing the complete prompt text and
update every assert.Contains / assert.NotContains that checks the LocalStack
prompt (the assertion currently using "Set up a LocalStack profile for AWS CLI
and SDKs?") to reference that variable instead (affecting the assertions around
the locations noted, including the existing assertion and the ones at the other
mentioned ranges).
---
Nitpick comments:
In `@test/integration/awsconfig_test.go`:
- Around line 215-216: The integration test currently invokes the CLI via
exec.CommandContext(ctx, binaryPath(), "config", "profile"); update that (and
the other occurrence around the later block) to exercise the user-facing setup
entrypoint by either replacing or adding a command invocation that calls
exec.CommandContext(ctx, binaryPath(), "setup", "aws") so the integration test
runs the "setup aws" flow; ensure you preserve cmd.Env = baseEnv and any
assertions/cleanup so the new path is covered and assertions reference the same
expected outputs for the AWS setup flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 96bd03cf-01f7-4e71-9fb0-4fe0026cba6c
📒 Files selected for processing (12)
CLAUDE.mdcmd/config.gocmd/root.gocmd/setup.gointernal/awsconfig/awsconfig.gointernal/config/containers.gointernal/container/start.gointernal/ui/app.gointernal/ui/components/input_prompt_test.gointernal/ui/run_awsconfig.gotest/integration/awsconfig_test.gotest/integration/start_test.go
✅ Files skipped from review due to trivial changes (4)
- internal/ui/components/input_prompt_test.go
- internal/container/start.go
- internal/ui/app.go
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/config/containers.go
- cmd/root.go
- internal/ui/run_awsconfig.go
- cmd/setup.go
- internal/awsconfig/awsconfig.go
- cmd/config.go
gtsiolis
force-pushed
the
george/des-178
branch
from
c968f27 to
91c6623
Compare
Member
Author
|
Great point, @carole-lavillonniere! 💯 I was kinda hoping for some pushback on this one! 🙏 Liked the Rebased and added that, updated also PR description. 🐰 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/awsconfig/awsconfig.go (1)
223-237:⚠️ Potential issue | 🟠 MajorPropagate real failures from
Setup.
Setupstill converts path/write failures into warnings and returnsnil, solstk setup awscan exit successfully after doing nothing or after only one of the two files was written. Keep the warning downgrade inEnsureProfile, wherelstk startis intentionally best-effort, and let the standalone setup command fail.Suggested fix
func EnsureProfile(ctx context.Context, sink output.Sink, interactive bool, resolvedHost string) error { status, configOK, credsOK, err := checkProfileSetup(resolvedHost) if err != nil { output.EmitWarning(sink, fmt.Sprintf("could not check AWS profile: %v", err)) return nil } if !status.anyNeeded() { return nil } if interactive && !configOK && !credsOK { - return Setup(ctx, sink, resolvedHost, status) + if err := Setup(ctx, sink, resolvedHost, status); err != nil { + output.EmitWarning(sink, err.Error()) + } + return nil } emitMissingProfileNote(sink) return nil } ... configPath, credsPath, err := awsPaths() if err != nil { - output.EmitWarning(sink, fmt.Sprintf("could not determine AWS config paths: %v", err)) - return nil + return fmt.Errorf("could not determine AWS config paths: %w", err) } ... if status.configNeeded { if err := writeConfigProfile(configPath, resolvedHost); err != nil { - output.EmitWarning(sink, fmt.Sprintf("could not update ~/.aws/config: %v", err)) - return nil + return fmt.Errorf("could not update ~/.aws/config: %w", err) } } if status.credsNeeded { if err := writeCredsProfile(credsPath); err != nil { - output.EmitWarning(sink, fmt.Sprintf("could not update ~/.aws/credentials: %v", err)) - return nil + return fmt.Errorf("could not update ~/.aws/credentials: %w", err) } }Also applies to: 249-280
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/awsconfig/awsconfig.go` around lines 223 - 237, The current EnsureProfile flow swallows real Setup failures (Setup converts path/write errors into warnings and returns nil), so update EnsureProfile and similar flow (the other block handling interactive setup around lines 249-280) to propagate non-nil errors returned by Setup when called as the standalone interactive setup (i.e., when interactive && !configOK && !credsOK), by returning the error from Setup instead of discarding it; also change Setup (function Setup) to return real errors for path/write failures (do not convert them into warnings inside Setup) so callers like the standalone `lstk setup aws` can fail cleanly, while keeping the existing warning downgrade behavior in EnsureProfile for non-standalone best-effort callers.
🧹 Nitpick comments (1)
internal/ui/run.go (1)
86-88: Fix therunWithTUIcontract comment.The helper already sends
runDoneMsg/runErrMsgon Lines 97-105, so the current docstring tells future callers to do the opposite and invites double completion messages.Suggested wording
-// runWithTUI runs a TUI command with the given app configuration. -// The runFn is executed in a goroutine and should send runDoneMsg or runErrMsg to the program. -// If the app ends with an error, it's wrapped in SilentError to suppress duplicate output. +// runWithTUI runs a TUI command with the given app configuration. +// runFn executes in a goroutine; runWithTUI forwards its completion to the +// program via runDoneMsg/runErrMsg. +// If the app ends with an error, it's wrapped in SilentError to suppress +// duplicate output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/ui/run.go` around lines 86 - 88, The comment for runWithTUI incorrectly instructs callers to send runDoneMsg/runErrMsg; update the docstring to reflect the actual contract: runWithTUI spawns the runFn and the helper itself sends runDoneMsg or runErrMsg (on completion or error) so callers should not emit those messages to avoid duplicate completion events. Edit the comment around runWithTUI to clearly state that runFn should perform work and return/propagate errors, while runWithTUI handles sending runDoneMsg/runErrMsg and wrapping terminal errors in SilentError.
🤖 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/awsconfig/awsconfig.go`:
- Around line 283-289: The success messages in the conditional that checks
status.configNeeded and status.credsNeeded use "Created ..." or "Updated ..."
but status only indicates which files/sections were written, not whether they
were created or changed; change the strings passed to output.EmitSuccess (the
branches handling status.configNeeded && status.credsNeeded,
status.configNeeded, and the else branch for creds) to neutral copy such as
"Wrote LocalStack profile in ~/.aws", "Wrote LocalStack profile in
~/.aws/config", and "Wrote LocalStack credentials in ~/.aws/credentials" (or
"Wrote/updated ..." if you prefer) so the messages accurately reflect repair
flows without implying creation.
---
Duplicate comments:
In `@internal/awsconfig/awsconfig.go`:
- Around line 223-237: The current EnsureProfile flow swallows real Setup
failures (Setup converts path/write errors into warnings and returns nil), so
update EnsureProfile and similar flow (the other block handling interactive
setup around lines 249-280) to propagate non-nil errors returned by Setup when
called as the standalone interactive setup (i.e., when interactive && !configOK
&& !credsOK), by returning the error from Setup instead of discarding it; also
change Setup (function Setup) to return real errors for path/write failures (do
not convert them into warnings inside Setup) so callers like the standalone
`lstk setup aws` can fail cleanly, while keeping the existing warning downgrade
behavior in EnsureProfile for non-standalone best-effort callers.
---
Nitpick comments:
In `@internal/ui/run.go`:
- Around line 86-88: The comment for runWithTUI incorrectly instructs callers to
send runDoneMsg/runErrMsg; update the docstring to reflect the actual contract:
runWithTUI spawns the runFn and the helper itself sends runDoneMsg or runErrMsg
(on completion or error) so callers should not emit those messages to avoid
duplicate completion events. Edit the comment around runWithTUI to clearly state
that runFn should perform work and return/propagate errors, while runWithTUI
handles sending runDoneMsg/runErrMsg and wrapping terminal errors in
SilentError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 46ff5891-5252-4fcc-8fed-47951c400a7c
📒 Files selected for processing (14)
CLAUDE.mdcmd/config.gocmd/root.gocmd/setup.gointernal/awsconfig/awsconfig.gointernal/awsconfig/awsconfig_test.gointernal/config/containers.gointernal/container/start.gointernal/ui/app.gointernal/ui/components/input_prompt_test.gointernal/ui/run.gointernal/ui/run_awsconfig.gotest/integration/awsconfig_test.gotest/integration/start_test.go
✅ Files skipped from review due to trivial changes (4)
- test/integration/start_test.go
- internal/ui/components/input_prompt_test.go
- CLAUDE.md
- internal/container/start.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/config/containers.go
- cmd/root.go
- cmd/config.go
- cmd/setup.go
- internal/ui/run_awsconfig.go
Comment on lines
+283
to
+289
| if status.configNeeded && status.credsNeeded { | ||
| output.EmitSuccess(sink, "Created LocalStack profile in ~/.aws") | ||
| } else if status.configNeeded { | ||
| output.EmitSuccess(sink, "Created LocalStack profile in ~/.aws/config") | ||
| } else { | ||
| output.EmitSuccess(sink, "Updated LocalStack credentials in ~/.aws/credentials") | ||
| } |
There was a problem hiding this comment.
Use neutral success copy for repair flows.
profileStatus only tells you what needs writing, not whether the write will create or update an existing section. In lstk setup aws, these branches can therefore print "Created ..." for an update path, or "Updated ..." when the credentials section is being created.
Suggested wording
- if status.configNeeded && status.credsNeeded {
- output.EmitSuccess(sink, "Created LocalStack profile in ~/.aws")
- } else if status.configNeeded {
- output.EmitSuccess(sink, "Created LocalStack profile in ~/.aws/config")
- } else {
- output.EmitSuccess(sink, "Updated LocalStack credentials in ~/.aws/credentials")
- }
+ if status.configNeeded && status.credsNeeded {
+ output.EmitSuccess(sink, "Configured LocalStack AWS profile in ~/.aws")
+ } else if status.configNeeded {
+ output.EmitSuccess(sink, "Configured LocalStack AWS profile in ~/.aws/config")
+ } else {
+ output.EmitSuccess(sink, "Configured LocalStack AWS profile in ~/.aws/credentials")
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/awsconfig/awsconfig.go` around lines 283 - 289, The success messages
in the conditional that checks status.configNeeded and status.credsNeeded use
"Created ..." or "Updated ..." but status only indicates which files/sections
were written, not whether they were created or changed; change the strings
passed to output.EmitSuccess (the branches handling status.configNeeded &&
status.credsNeeded, status.configNeeded, and the else branch for creds) to
neutral copy such as "Wrote LocalStack profile in ~/.aws", "Wrote LocalStack
profile in ~/.aws/config", and "Wrote LocalStack credentials in
~/.aws/credentials" (or "Wrote/updated ..." if you prefer) so the messages
accurately reflect repair flows without implying creation.
carole-lavillonniere
approved these changes
Mar 24, 2026
| } | ||
| } | ||
| return nil | ||
| } |
Collaborator
There was a problem hiding this comment.
Looks like this is still not addressed?
Member
Author
|
Thanks for taking another look, @carole-lavillonniere! Tried to address review round comments in a5ebba4. |
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
4 participants
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.
This PR improves the AWS profile setup flow to reduce repeated prompts during
lstk startwithout adding extra persisted config state.Users running lstk start repeatedly were prompted every time even when a partial profile existed. This change makes the prompt conditional while still guiding users to set up a complete profile via lstk setup aws.
lstk startnow prompts only when the localstack profile is missing from both~/.aws/configand~/.aws/credentialslstk setup awscommand for interactive AWS profile setup