Conversation
carole-lavillonniere
changed the title
carole-lavillonniere
force-pushed
the
carole/drg-565
branch
from
d605869 to
a02b1ca
Compare
carole-lavillonniere
force-pushed
the
carole/drg-565
branch
from
a02b1ca to
b3b4457
Compare
carole-lavillonniere
requested review from
anisaoshafi and
silv-io
as code owners
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThreads a LocalStackHost value through CLI/UI/container flows, adds DNS-resilient LocalStack host resolution, runs per-emulator post-start setups (AWS profile setup), implements awsconfig and INI helpers, and adds unit and integration tests for AWS profile management. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant UI as internal/ui.Run
participant Container as internal/container.Start
participant Orch as Post-Start Orchestrator
participant AWSSetup as internal/awsconfig.Setup
participant Files as "~/.aws files"
User->>UI: Start(localStackHost)
UI->>Container: Start(..., localStackHost)
Container->>Container: launch containers
Container->>Orch: runPostStartSetups(types, firstContainer, localStackHost)
Orch->>AWSSetup: Setup(ctx, sink, interactive, port, localStackHost)
AWSSetup->>Files: Check config & credentials
alt missing or stale profile
AWSSetup->>User: Prompt (if interactive)
User-->>AWSSetup: Confirm
AWSSetup->>Files: Write/update profile sections
end
AWSSetup-->>Orch: result
Orch-->>Container: post-start complete
Container-->>User: Ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 (2)
internal/awsconfig/awsconfig.go (1)
25-28: Avoid a mutable package-level map for credential defaults.
credentialsKeysis shared mutable state inside a domain package. Keeping these expected pairs local to the call sites, or returning a fresh map from a helper, removes the hidden global dependency.As per coding guidelines, "Avoid package-level global variables. Use constructor functions that return fresh instances and inject dependencies explicitly."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/awsconfig/awsconfig.go` around lines 25 - 28, The package-level mutable map credentialsKeys should be removed and replaced with a factory that returns a fresh map or by inlining the literal at call sites; create a function (e.g., newCredentialsKeys or buildCredentialsDefaults) that returns map[string]string{"aws_access_key_id":"test","aws_secret_access_key":"test"} and update all usages to call that function so each caller gets a fresh instance instead of sharing global state (or move the literal into the local scope where it's used), ensuring no package-level mutable variables remain.test/integration/awsconfig_test.go (1)
77-81: Assert both credential lines explicitly.This still passes if only
aws_access_key_id = testis written. Checkingaws_secret_access_key = testas well makes the test catch incomplete credentials files.Suggested assertion tightening
assert.Contains(t, string(credsContent), "[localstack]") - assert.Contains(t, string(credsContent), "aws_access_key_id") - assert.Contains(t, string(credsContent), "= test") + assert.Contains(t, string(credsContent), "aws_access_key_id = test") + assert.Contains(t, string(credsContent), "aws_secret_access_key = test")🤖 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 77 - 81, The test currently only asserts that the credentials file contains "aws_access_key_id" and a generic "= test"; update the assertions to explicitly check for both credential lines by asserting credsContent (from reading filepath.Join(tmpHome, ".aws", "credentials")) contains "aws_access_key_id = test" and also contains "aws_secret_access_key = test" so the test fails if either key is missing or incomplete.
🤖 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 30-45: The isValidLocalStackEndpoint function currently checks
fixed string prefixes and misses non-default LOCALSTACK_HOST values and
malformed ports; update it to parse the input URL (using net/url.Parse) and
compare the parsed host and port against the resolved host and port produced by
Setup (use the same resolved hostname/port variables that Setup returns or
stores), i.e., extract u.Hostname() and u.Port() and validate equality with
Setup's host and port rather than relying on prefix matching; also return false
on parse errors or missing port to avoid accepting malformed endpoints.
In `@internal/awsconfig/ini.go`:
- Around line 28-49: upsertSection currently writes credentials via
ini.File.SaveTo(path) which can create a world-readable file; after saving the
file call os.Chmod(path, 0600) and return any chmod error (i.e., after
f.SaveTo(path) add err = os.Chmod(path, 0600) and handle it), or alternatively
open the destination with os.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_TRUNC,
0600) and write the INI contents to that file so the created file has explicit
0600 permissions; ensure errors from Chmod or OpenFile/write are returned so
upsertSection fails on permission issues.
In `@test/integration/awsconfig_test.go`:
- Around line 117-135: The test asserts absence of the profile prompt too early;
move the NotContains assertion to after the PTY reader finishes draining so we
check the final output. Keep the existing require.Eventually check for
bytes.Contains(out.Bytes(), []byte(" ready")) but change the sequence so you
call cmd.Process.Kill(), cmd.Wait(), then block on <-outputCh (the goroutine
that does io.Copy from ptmx into the syncBuffer out) and only after that run
assert.NotContains(t, out.String(), "Set up LocalStack AWS profile") to ensure
the post-start AWS profile check had time to run.
---
Nitpick comments:
In `@internal/awsconfig/awsconfig.go`:
- Around line 25-28: The package-level mutable map credentialsKeys should be
removed and replaced with a factory that returns a fresh map or by inlining the
literal at call sites; create a function (e.g., newCredentialsKeys or
buildCredentialsDefaults) that returns
map[string]string{"aws_access_key_id":"test","aws_secret_access_key":"test"} and
update all usages to call that function so each caller gets a fresh instance
instead of sharing global state (or move the literal into the local scope where
it's used), ensuring no package-level mutable variables remain.
In `@test/integration/awsconfig_test.go`:
- Around line 77-81: The test currently only asserts that the credentials file
contains "aws_access_key_id" and a generic "= test"; update the assertions to
explicitly check for both credential lines by asserting credsContent (from
reading filepath.Join(tmpHome, ".aws", "credentials")) contains
"aws_access_key_id = test" and also contains "aws_secret_access_key = test" so
the test fails if either key is missing or incomplete.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4cb6a292-a8c7-4d5a-b552-fad73196ebeb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
cmd/root.gogo.modinternal/awsconfig/awsconfig.gointernal/awsconfig/awsconfig_test.gointernal/awsconfig/ini.gointernal/container/start.gointernal/container/start_test.gointernal/endpoint/endpoint.gointernal/env/env.gointernal/ui/run.gotest/integration/awsconfig_test.gotest/integration/env/env.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/integration/awsconfig_test.go (1)
101-107: Pre-written endpoint URL may not match resolved host, causing test flakiness.The pre-configured
endpoint_url = http://127.0.0.1:4566might not match the actual resolved LocalStack host. Per the PR description, the endpoint package falls back to127.0.0.1only when DNS rebind protection blockslocalhost.localstack.cloud. If DNS resolution succeeds in the CI/test environment, the resolved host would belocalhost.localstack.cloud:4566, triggering a config update prompt and failing the assertion.Consider using the same host resolution logic or a deterministic endpoint that will always match:
require.NoError(t, os.WriteFile(filepath.Join(awsDir, "config"), - []byte("[profile localstack]\nregion = us-east-1\noutput = json\nendpoint_url = http://127.0.0.1:4566\n"), 0600)) + []byte("[profile localstack]\nregion = us-east-1\noutput = json\nendpoint_url = http://localhost.localstack.cloud:4566\n"), 0600))Alternatively, set
LOCALSTACK_HOST=127.0.0.1:4566in the test environment to force consistent host resolution.🤖 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 101 - 107, The test writes a hardcoded endpoint_url (http://127.0.0.1:4566) into the AWS config which can mismatch the runtime-resolved LocalStack host and make the test flaky; update the pre-written config generation (the block that builds awsDir using tmpHome and writes the "config" file) to use the same host resolution as production (either call the project’s LocalStack host resolver function or read LOCALSTACK_HOST) and write endpoint_url using that resolved host, or alternatively set LOCALSTACK_HOST=127.0.0.1:4566 for the test environment so the pre-written config (the file written via filepath.Join(awsDir, "config")) always matches the resolved host.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/integration/awsconfig_test.go`:
- Around line 101-107: The test writes a hardcoded endpoint_url
(http://127.0.0.1:4566) into the AWS config which can mismatch the
runtime-resolved LocalStack host and make the test flaky; update the pre-written
config generation (the block that builds awsDir using tmpHome and writes the
"config" file) to use the same host resolution as production (either call the
project’s LocalStack host resolver function or read LOCALSTACK_HOST) and write
endpoint_url using that resolved host, or alternatively set
LOCALSTACK_HOST=127.0.0.1:4566 for the test environment so the pre-written
config (the file written via filepath.Join(awsDir, "config")) always matches the
resolved host.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: c8e4055c-50ad-4b5e-84dd-dccc388a3723
📒 Files selected for processing (4)
internal/awsconfig/awsconfig.gointernal/awsconfig/awsconfig_test.gointernal/awsconfig/ini.gotest/integration/awsconfig_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/awsconfig/awsconfig.go
carole-lavillonniere
force-pushed
the
carole/drg-565
branch
2 times, most recently
from
35e8996 to
bfbb649
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/integration/awsconfig_test.go (1)
124-128:⚠️ Potential issue | 🟡 MinorPotential race:
" ready"matches the early log line, not post-start completion.The substring
" ready"appears in"Waiting for localstack-aws to be ready..."which is logged before the container is actually ready and before post-start setup runs. This could cause the test to proceed and kill the process before the AWS profile check has a chance to run (or not run).Consider waiting for a more specific marker like
"localstack-aws ready"or the full"ready (containerId:"pattern to ensure post-start setup has completed.Suggested fix
require.Eventually(t, func() bool { - return bytes.Contains(out.Bytes(), []byte(" ready")) + return bytes.Contains(out.Bytes(), []byte("localstack-aws ready")) }, 2*time.Minute, 200*time.Millisecond, "container should become ready")🤖 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 124 - 128, The test currently waits for a generic substring by calling require.Eventually with bytes.Contains(out.Bytes(), []byte(" ready")), which can match the early log "Waiting for localstack-aws to be ready..." and cause a race; update the predicate used in the require.Eventually call (the bytes.Contains check) to look for a more specific post-start completion marker such as "localstack-aws ready" or the full "ready (containerId:" pattern so the wait only succeeds after post-start setup has finished (modify the predicate in the require.Eventually block in test/integration/awsconfig_test.go).
🧹 Nitpick comments (1)
internal/awsconfig/awsconfig_test.go (1)
222-257: Add test case for "neither needed" scenario.The
profileStatus{configNeeded: false, credsNeeded: false}case is missing. WhilefilesToModify()likely returns an empty slice, explicitly testing this documents the expected behavior and guards against regressions.Suggested addition
tests := []struct { name string status profileStatus wantFiles []string }{ + { + name: "neither needed", + status: profileStatus{configNeeded: false, credsNeeded: false}, + wantFiles: []string{}, + }, { name: "both needed",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/awsconfig/awsconfig_test.go` around lines 222 - 257, Add a test case to TestFilesToModify for the scenario profileStatus{configNeeded:false, credsNeeded:false} that asserts filesToModify() returns an empty slice; update the tests table (in TestFilesToModify) to include a new entry with name "neither needed" and wantFiles set to []string{}, and in the existing loop the current length and element checks will verify the empty result (or you can explicitly assert len(got)==0) so the behavior of profileStatus.filesToModify is documented and guarded against regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/integration/awsconfig_test.go`:
- Around line 124-128: The test currently waits for a generic substring by
calling require.Eventually with bytes.Contains(out.Bytes(), []byte(" ready")),
which can match the early log "Waiting for localstack-aws to be ready..." and
cause a race; update the predicate used in the require.Eventually call (the
bytes.Contains check) to look for a more specific post-start completion marker
such as "localstack-aws ready" or the full "ready (containerId:" pattern so the
wait only succeeds after post-start setup has finished (modify the predicate in
the require.Eventually block in test/integration/awsconfig_test.go).
---
Nitpick comments:
In `@internal/awsconfig/awsconfig_test.go`:
- Around line 222-257: Add a test case to TestFilesToModify for the scenario
profileStatus{configNeeded:false, credsNeeded:false} that asserts
filesToModify() returns an empty slice; update the tests table (in
TestFilesToModify) to include a new entry with name "neither needed" and
wantFiles set to []string{}, and in the existing loop the current length and
element checks will verify the empty result (or you can explicitly assert
len(got)==0) so the behavior of profileStatus.filesToModify is documented and
guarded against regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 901ee45c-c8f5-487e-a177-dd38cdcb7e64
📒 Files selected for processing (6)
cmd/non_interactive_test.gointernal/awsconfig/awsconfig.gointernal/awsconfig/awsconfig_test.gointernal/awsconfig/ini.gointernal/ui/app_test.gotest/integration/awsconfig_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/awsconfig/awsconfig.go
- internal/awsconfig/ini.go
carole-lavillonniere
force-pushed
the
carole/drg-565
branch
from
bfbb649 to
1024e41
Compare
anisaoshafi
left a comment
anisaoshafi
left a comment
There was a problem hiding this comment.
LGTM, my manual tests worked as expected 🎉
| } | ||
| // Use a "test." subdomain: *.localhost.localstack.cloud has wildcard DNS that resolves | ||
| // to 127.0.0.1, so any subdomain works as a probe without hitting the actual service. | ||
| addrs, err := net.LookupHost("test." + Hostname) |
There was a problem hiding this comment.
small FYI: I had endpoint_url http://127.0.0.1:4566 in my profile, but it asked me to override the profile (with http://localhost.localstack.cloud:4566), if I skip it it will keep asking me each time I start lstk again.
I was wondering if we want to prompt the user to override their profile if they have http://127.0.0.1:4566 or http://localhost which also work fine (if I remember correctly localstack toolkit doesn't prompt in this case).
I'm fine with how this looks now, no need to overcomplicate the code by adding another condition.
| if isInteractiveMode(cfg) { | ||
| return ui.Run(ctx, rt, version.Version(), platformClient, cfg.AuthToken, cfg.ForceFileKeyring, cfg.WebAppURL) | ||
| return ui.Run(ctx, rt, version.Version(), platformClient, cfg.AuthToken, cfg.ForceFileKeyring, cfg.WebAppURL, cfg.LocalStackHost) | ||
| } | ||
| return container.Start(ctx, rt, output.NewPlainSink(os.Stdout), platformClient, cfg.AuthToken, cfg.ForceFileKeyring, cfg.WebAppURL, false) | ||
| return container.Start(ctx, rt, output.NewPlainSink(os.Stdout), platformClient, cfg.AuthToken, cfg.ForceFileKeyring, cfg.WebAppURL, false, cfg.LocalStackHost) | ||
| } |
There was a problem hiding this comment.
thought: Both of these have quite a long list of parameters now. Is there a way to do this more nicely? 🤔
| } | ||
|
|
||
| var dnsOK bool | ||
| localStackHost, dnsOK = endpoint.ResolveHost(port, localStackHost) |
There was a problem hiding this comment.
nit: we're resolving this also in the ui.Run. Maybe we can unify that somehow?
| } | ||
| } | ||
| output.EmitSuccess(sink, fmt.Sprintf("LocalStack AWS profile written to %s", files)) | ||
| output.EmitNote(sink, fmt.Sprintf("Try: aws s3 mb s3://test --profile %s", profileName)) |
There was a problem hiding this comment.
praise: YASSS! ❤️ This will resolve DRG-363.
4 participants
Auto-configure AWS credentials for new users to immediately execute AWS CLI commands against LocalStack.
Closes DRG-565
internal/awsconfigpackage to manage the LocalStack AWS profile in~/.aws/configand~/.aws/credentialsinternal/endpointpackage with DNS-aware host resolution (falls back to127.0.0.1when DNS rebind protection blockslocalhost.localstack.cloud). Can be overwritten with env varLOCALSTACK_HOSTfor consistency with containercontainer/start.goto initiate the profile setup automatically onlstk start: prompts in interactive mode, emits a note in non-interactive mode~/.aws/configand/or~/.aws/credentials) will be modified