Forward host environment variables to the emulator container#161
Forward host environment variables to the emulator container#161
Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere 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 (3)
📝 WalkthroughWalkthroughHost environment variables with prefix Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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: 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/container/start_test.go`:
- Line 72: Replace the unchecked defer os.Setenv("CI", originalCI) with a
t.Cleanup that restores the env and handles errors: inside the test (in
start_test.go) register t.Cleanup(func() { if err := os.Setenv("CI",
originalCI); err != nil { t.Fatalf("failed to restore CI env: %v", err) } }) so
the environment is restored reliably and the error from os.Setenv is not
ignored; alternatively, if available use t.Setenv to manage the variable
automatically.
- Around line 85-103: The test is rebuilding the env-forwarding logic instead of
exercising the real implementation and also ignores the os.Setenv error; extract
the host environment collection into a testable function (e.g., collectHostEnv
in start.go) that iterates os.Environ() and filters entries with
strings.HasPrefix(e, "CI=") or the LOCALSTACK_ rules, update Start to call
collectHostEnv, then rewrite the test to call collectHostEnv directly (in
start_test.go) and assert presence/absence of "CI=" using strings.HasPrefix
checks; also handle os.Setenv/os.Unsetenv return values (check and/or use
t.Cleanup and os.LookupEnv to restore original state) so no env-setting errors
are ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 892049f8-b49a-4036-8d20-0d575e08580c
📒 Files selected for processing (2)
internal/container/start.gointernal/container/start_test.go
gtsiolis
force-pushed
the
george/des-184
branch
from
d784aba to
1384ec6
Compare
| defer func() { _ = resp.Body.Close() }() | ||
| assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
| }) | ||
| } |
There was a problem hiding this comment.
issue: can we cover the new functionality with an integration test? It could set CI and another LOCALSTACK_* env var, and make sure it's passed to the container. In case we change the value of CI, let's reset it to its original value after the test is done.
gtsiolis
force-pushed
the
george/des-184
branch
from
56a6a8f to
2d8e0ec
Compare
| require.NoError(t, err) | ||
| defer resp.Body.Close() | ||
| assert.Equal(t, http.StatusOK, resp.StatusCode) | ||
| }) |
There was a problem hiding this comment.
issue: why are we removing all these subtests?
- docker socket mount
- service port range
- main port
- https port
- volume mount
- http health endpoint
- https health endpoint
| assert.Equal(t, tt.expected, got) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
issue: I don't see the point of these unit tests. They are duplicating the logic of the code under test. I think we can delete and rely solely on the new integration tests.
| t.Setenv("CI", "true") | ||
| t.Setenv("LOCALSTACK_DISABLE_EVENTS", "1") | ||
| t.Setenv("LOCALSTACK_AUTH_TOKEN", "host-token") |
There was a problem hiding this comment.
issue: we should not be setting env vars like this, instead when running the lstk command:
_, stderr, err := runLstk(t, ctx, "",
env.With(env.APIEndpoint, mockServer.URL).
With(env.CI, "true").
With(env.DisableEvents, "1").
With(env.AuthToken, "host-token"),
"start")
With env.With we inject the env vars explicitly into the subprocess env without mutating the test process env, it is cleaner and consistent with how other tests work.
2 participants
Forwards two sets of host environment variables into the LocalStack container at startup, matching the behaviour of the legacy CLI:
CI: ensures LocalStack behaves correctly in CI environments.LOCALSTACK_*: forwards all LocalStack configuration variables set on the host. LOCALSTACK_AUTH_TOKEN is excluded since it is already explicitly injected.