Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
📝 WalkthroughWalkthroughAdded a new Changes
Sequence DiagramsequenceDiagram
participant User as User
participant LSCmd as lstk aws
participant Config as Config/Container
participant AWSCli as AWS CLI
participant Spinner as Spinner
User->>LSCmd: lstk aws [args...]
LSCmd->>Config: resolveAWSPort()
Config-->>LSCmd: port
LSCmd->>LSCmd: Resolve LocalStack endpoint<br/>(host:port)
LSCmd->>LSCmd: BuildEnv() for AWS credentials
alt Terminal Detected
LSCmd->>Spinner: Start spinner
end
LSCmd->>AWSCli: Exec with<br/>--endpoint-url + args
AWSCli->>AWSCli: Execute command
alt Output Received
AWSCli-->>Spinner: Write to stdout/stderr
Spinner->>Spinner: Stop on first write
end
AWSCli-->>LSCmd: Exit status
LSCmd-->>User: Output + exit code
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
cmd/aws.go (1)
15-25: Align user-facing help text with emulator terminology guidance.Please update the command help to reference emulator terminology/types instead of only “LocalStack” phrasing.
As per coding guidelines, CLI/help/docs in
cmd/**/*.goshould reference “emulator types (aws, snowflake, azure)” with only aws currently implemented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/aws.go` around lines 15 - 25, Update the user-facing help text for the AWS CLI command (the cobra command variable like awsCmd / the Short and Long strings) to use the emulator terminology: refer to the emulator type "aws" (and note emulator types such as aws, snowflake, azure) instead of only "LocalStack"; keep existing example showing endpoint and env vars but change phrases like "Proxy AWS CLI commands to LocalStack" to "Proxy AWS CLI commands to the aws emulator (LocalStack)" and mention that only the aws emulator type is currently implemented. Ensure both Short and Long strings are updated accordingly to follow cmd/**/*.go guidance.
🤖 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/aws.go`:
- Around line 44-47: The loop over appCfg.Containers returning c.Port for the
AWS emulator can return an empty string; update the logic in the function that
iterates appCfg.Containers (checking c.Type == config.EmulatorAWS) to guard that
c.Port is not empty before returning it—i.e., only return c.Port when both the
container type equals config.EmulatorAWS and c.Port != ""; otherwise continue
searching and if no non-empty port is found, return the configured default port
value (use the existing default constant or literal used elsewhere in the file).
In `@internal/awscli/exec.go`:
- Around line 17-27: The spinner can be stopped twice (one per stdout/stderr
wrapper) and may never be stopped if the subprocess emits no output; fix by
making Stop idempotent and by sharing a single stop action between writers and
invoking it when the process exits. Update the spinner type to guard its Stop
method with an internal sync.Once (make spinner.Stop safe for repeated calls),
and change stopOnWriteWriter construction so both instances receive the same
stop function (or a shared sync.Once) instead of calling spinner.Stop directly
in Write; also ensure the code that waits for the subprocess explicitly calls
that shared stop function after process completion so the spinner is always
stopped even when there is no output (referencing stopOnWriteWriter, its Write
method, and spinner.Stop).
In `@test/integration/aws_cmd_test.go`:
- Around line 42-43: Tests in aws_cmd_test.go currently use runLstk (non-TTY)
and a fake aws binary that only echoes, so spinner lifecycle (TTY activation and
stopping on first streamed output) isn't exercised; add a new PTY-based
integration test that uses runLstkInPTY and a fake aws binary which streams
output (writes something after a short delay) to validate that the spinner
appears while waiting and is cleared once the first output is streamed. Create a
new test function (e.g., TestAwsCmd_SpinnerLifecyclePTY) that calls
runLstkInPTY(testContext(t), t.TempDir(), e, "aws", "s3", "ls") with the
streaming fake binary and assert PTY output contains spinner characters prior to
streamed content and that spinner is removed once output begins; leave existing
runLstk-based tests (argument injection and credential propagation) unchanged.
---
Nitpick comments:
In `@cmd/aws.go`:
- Around line 15-25: Update the user-facing help text for the AWS CLI command
(the cobra command variable like awsCmd / the Short and Long strings) to use the
emulator terminology: refer to the emulator type "aws" (and note emulator types
such as aws, snowflake, azure) instead of only "LocalStack"; keep existing
example showing endpoint and env vars but change phrases like "Proxy AWS CLI
commands to LocalStack" to "Proxy AWS CLI commands to the aws emulator
(LocalStack)" and mention that only the aws emulator type is currently
implemented. Ensure both Short and Long strings are updated accordingly to
follow cmd/**/*.go guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2e1d4c43-f552-4aa8-b33a-bb1ff4a6edc1
📒 Files selected for processing (9)
cmd/aws.gocmd/root.gointernal/awscli/exec.gointernal/awscli/exec_test.gointernal/awscli/spinner.gointernal/output/errors.gomain.gotest/integration/aws_cmd_test.gotest/integration/start_test.go
There was a problem hiding this comment.
Thanks George for investing in this nice feature ⭐
- Issue: The spinner behaviour is weird, it gets stuck e.g:
- a) when you try
lstk aws s3 lswhen you have no bucket - b) create a bucket and then delete bucket. I suggest to revisit it to make it more robust 🙏🏼
- Suggestion: would be nice to add a more descriptive message when LocalStack is not running:
| "strings" | ||
| "sync" | ||
| "time" | ||
| ) | ||
|
|
||
| var dotFrames = []string{"⣾", "⣽", "⣻", "⢿", "⡿", "⣟", "⣯", "⣷"} | ||
|
|
||
| // ANSI color codes matching lstk's spinner style (color 69 blue) and secondary (color 241 gray) | ||
| const ( | ||
| spinnerColor = "\033[38;5;69m" | ||
| secondaryColor = "\033[38;5;241m" | ||
| resetColor = "\033[0m" | ||
| ) |
There was a problem hiding this comment.
issue: didn't we have these properties defined somewhere for Nimbo? I think they're duplicated.
Also, In components package, we have spinner defined. Why are we redefining it again in this PR, can you try reuse them?
Btw 🎗️, we have this /review-pr claude skill: /review-pr #176 that the team added, and it can be run in local development to catch this architectural pattern issues 🙏🏼
There was a problem hiding this comment.
Hopefully we can reuse the color but I don't think we can reuse the spinner.
The one we have requires Bubble tea and here we don't have Bubble tea because we are just passing through stdout/stderr.
I think a new spinner cannot be avoided, but maybe we can move it to a more generic place such as internal/terminal so it can be reused for other commands in the future.
|
@gtsiolis the PR title and description are confusing to me. Aren't we adding a brand new |
| "strings" | ||
| "sync" | ||
| "time" | ||
| ) | ||
|
|
||
| var dotFrames = []string{"⣾", "⣽", "⣻", "⢿", "⡿", "⣟", "⣯", "⣷"} | ||
|
|
||
| // ANSI color codes matching lstk's spinner style (color 69 blue) and secondary (color 241 gray) | ||
| const ( | ||
| spinnerColor = "\033[38;5;69m" | ||
| secondaryColor = "\033[38;5;241m" | ||
| resetColor = "\033[0m" | ||
| ) |
There was a problem hiding this comment.
Hopefully we can reuse the color but I don't think we can reuse the spinner.
The one we have requires Bubble tea and here we don't have Bubble tea because we are just passing through stdout/stderr.
I think a new spinner cannot be avoided, but maybe we can move it to a more generic place such as internal/terminal so it can be reused for other commands in the future.
|
|
||
| cmd := exec.CommandContext(ctx, awsBin, cmdArgs...) | ||
| cmd.Stdin = os.Stdin | ||
| cmd.Env = BuildEnv(os.Environ()) |
There was a problem hiding this comment.
question: we already setup the aws profile as part of lstk start. Did we add this so it works when user has not run lstk start yet? Can we instead reuse that logic instead of reinventing it?
| return exitErr.Code | ||
| } | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
issue: this new ExitCodeError struct does not seem needed. exec.ExitError (from go standard lib, returned by cmd.Run() when the process exits with a non-zero status) already carries the exit code via .ExitCode().
I think the whole ExitCodeError type and output.ExitCode() helper can be dropped.
You'd have to do this in main.go:
- os.Exit(output.ExitCode(err))
+ var exitErr *exec.ExitError
+ if errors.As(err, &exitErr) {
+ os.Exit(exitErr.ExitCode())
+ }
+ os.Exit(1)
And this in internal/awscli/exec.go:
- if errors.As(err, &exitErr) {
- return output.NewSilentError(output.NewExitCodeError(exitErr.ExitCode(), err))
- }
+ if errors.As(err, &exitErr) {
+ return output.NewSilentError(err)
+ }
gtsiolis
force-pushed
the
des-190-replace-aws-cli-profile-option-with-lstk-prefix
branch
from
5bcff25 to
abcbfc3
Compare
Great catch, @anisaoshafi! Removed the loading state when no buckets are there and when deleting buckets to recreate the UX of the AWS CLI itself.
Agree, we can have a better control of this, but kept it as is for now to recreate the AWS CLI UX. Thoughts? @carole-lavillonniere I pushed bf06946 to attempt to resolve the comments. 👀 |
gtsiolis
changed the title
3 participants
Updates the AWS CLI proxy command to show a loading spinner while waiting for a response, then automatically hide it as soon as output starts streaming.
See DES-190 and DRG-364 for more context.