Conversation
carole-lavillonniere
force-pushed
the
carole/drg-529
branch
from
69eb255 to
dc398d1
Compare
carole-lavillonniere
force-pushed
the
carole/drg-529
branch
from
4e14309 to
f090357
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughIntroduces a centralized environment package ( Changes
Sequence Diagram(s)mermaid 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 4
🧹 Nitpick comments (2)
internal/env/env.go (1)
16-16: Consider initializingVarsto a zero-valueEnvto prevent nil pointer panics.If any code accesses
env.VarsbeforeInit()is called, it will panic with a nil pointer dereference. While the intended usage is to callInit()early in the application lifecycle, a defensive initialization provides a safer default.💡 Suggested defensive initialization
-var Vars *Env +var Vars = &Env{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/env/env.go` at line 16, Vars is currently declared as a nil pointer (var Vars *Env) which can cause nil-pointer panics if accessed before Init(); change its declaration to a defensive zero-value pointer (e.g., initialize Vars to &Env{} at declaration) so callers can safely read default fields before Init() runs, and ensure Init() still overwrites or reinitializes Vars as needed; look for the Vars variable and the Init() function in the env package to make this change.test/integration/env/env.go (1)
65-68:With()appends without removing existing keys, which may not override as expected.When duplicate environment variable entries exist, behavior is platform-dependent—often the first occurrence takes precedence. If an entry for
keyalready exists ine, appending a new value won't reliably override it.Consider removing the existing key before appending:
♻️ Suggested fix to ensure override behavior
// With returns a copy of e with key=value appended. func (e Environ) With(key Key, value string) Environ { - return append(e, string(key)+"="+value) + return e.Without(key).append(string(key) + "=" + value) } + +func (e Environ) append(entry string) Environ { + return append(e, entry) +}Or more simply:
// With returns a copy of e with key=value set, replacing any existing value. func (e Environ) With(key Key, value string) Environ { - return append(e, string(key)+"="+value) + filtered := e.Without(key) + return append(filtered, string(key)+"="+value) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/env/env.go` around lines 65 - 68, The With method on Environ appends a new "key=value" entry but does not remove any existing entries for the same key, so duplicates may not override as expected; update Environ.With to filter out any existing entries whose prefix matches string(key)+"=" (using the Key type) and then append the new "key=value" entry so the returned Environ contains a single authoritative value for that key. Ensure you operate on the receiver Environ (e) and return the filtered slice with the new entry appended to preserve expected override semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@env.example`:
- Around line 1-11: The env example uses KEYRING but viper is configured with
the LOCALSTACK prefix and code calls viper.GetString("keyring"), so update the
example to export LOCALSTACK_KEYRING instead of KEYRING; change the commented
line in env.example (the KEYRING entry) to export LOCALSTACK_KEYRING (e.g. use
LOCALSTACK_KEYRING=file) so the env var matches viper's prefix and the
viper.GetString("keyring") call reads the intended value.
In `@internal/api/client.go`:
- Around line 68-71: NewPlatformClient dereferences env.Vars directly which can
panic if env.Init() hasn't run; update NewPlatformClient to guard against nil
env.Vars (or call a safe accessor) before using env.Vars.APIEndpoint and
fallback to a sensible default or trigger lazy env initialization; specifically
modify NewPlatformClient and any PlatformClient construction to use a nil-check
on env.Vars (or an env.GetAPIEndpoint()/env.MustInit() helper) so creating a
PlatformClient in tests or early library usage won't panic.
In `@internal/auth/token_storage.go`:
- Around line 48-50: Update the misleading inline comment above the KEYRING
check to reference the actual environment variable name used
(LOCALSTACK_KEYRING) so it matches env.Vars.Keyring; edit the comment near the
keyringConfig.AllowedBackends assignment in token_storage.go (the block that
checks env.Vars.Keyring == "file") to say "Force file backend if
LOCALSTACK_KEYRING env var is set to 'file'".
In `@test/integration/main_test.go`:
- Around line 65-67: Update the Makefile to set the new environment variable
name LOCALSTACK_KEYRING=file (replacing any KEYRING=file entry) so
env.Get(env.Keyring) in the test will see the file backend, and update the
comment near the test (around the env.Get(env.Keyring) check) to reference
LOCALSTACK_KEYRING instead of KEYRING; this ensures
keyringConfig.AllowedBackends = []keyring.BackendType{keyring.FileBackend} is
applied when running the integration tests.
---
Duplicate comments:
In `@internal/auth/auth.go`:
- Around line 38-39: The code reads env.Vars.AuthToken without guarding against
env.Vars being nil; update the accessor in internal/auth/auth.go to mirror the
central fix used in internal/api/client.go by checking that env.Vars is non-nil
before reading AuthToken (return a clear error if nil), or call the shared
helper that safely returns environment vars; ensure you reference env.Vars and
AuthToken when adding the nil-check so all env.Vars accessors follow the same
centralized pattern.
In `@internal/auth/login.go`:
- Around line 123-124: getWebAppURL currently returns env.Vars.WebAppURL without
a nil check; update getWebAppURL to guard against env.Vars being nil (same
central fix used in internal/api/client.go) by checking env.Vars and returning a
safe default (empty string or configured fallback) when nil, and apply the
identical nil-guard pattern to all other accessors that reference env.Vars to
ensure consistent behavior (identify accessors by function name such as
getWebAppURL and mirror the nil-check logic used in the api client fix).
---
Nitpick comments:
In `@internal/env/env.go`:
- Line 16: Vars is currently declared as a nil pointer (var Vars *Env) which can
cause nil-pointer panics if accessed before Init(); change its declaration to a
defensive zero-value pointer (e.g., initialize Vars to &Env{} at declaration) so
callers can safely read default fields before Init() runs, and ensure Init()
still overwrites or reinitializes Vars as needed; look for the Vars variable and
the Init() function in the env package to make this change.
In `@test/integration/env/env.go`:
- Around line 65-68: The With method on Environ appends a new "key=value" entry
but does not remove any existing entries for the same key, so duplicates may not
override as expected; update Environ.With to filter out any existing entries
whose prefix matches string(key)+"=" (using the Key type) and then append the
new "key=value" entry so the returned Environ contains a single authoritative
value for that key. Ensure you operate on the receiver Environ (e) and return
the filtered slice with the new entry appended to preserve expected override
semantics.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/env/env.go (2)
21-21: TheSetEnvKeyReplacerhas no effect with current key names.The replacer converts dots to underscores, but all your Viper keys already use underscores (
auth_token,api_endpoint, etc.). If this is intentional for future flexibility (e.g., hierarchical config likeapi.endpoint), consider adding a brief comment. Otherwise, it can be removed to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/env/env.go` at line 21, SetEnvKeyReplacer call is ineffective because Viper keys in this code use underscores (e.g., auth_token, api_endpoint); either remove the viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) call or add a brief comment above it clarifying it’s kept intentionally for future dot-to-underscore mapping (e.g., supporting hierarchical keys like api.endpoint) so readers aren’t confused; update the code around the viper.SetEnvKeyReplacer invocation (symbol: viper.SetEnvKeyReplacer) accordingly.
16-16: Consider documenting or enforcing initialization order.
Varsis initialized as an empty&Env{}and only populated whenInit()is called. Any code accessingVarsbeforeInit()will silently receive empty strings. Sincecmd/root.gocallsenv.Init()inPersistentPreRunE, this should be fine for the CLI flow, but it may be worth adding a brief doc comment noting thatInit()must be called before accessingVars.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/env/env.go` at line 16, Vars is currently created as an empty &Env{} and can be read before Init(), so add a clear doc comment above the package-level variable Vars stating that Init() must be called before any use; to harden further, change Vars to nil by default and add a small accessor function (e.g., GetVars() or MustVars()) that checks Vars != nil and returns it or panics/errors with a clear message, and update callers to use that accessor or ensure Init() is invoked early (Init(), Vars, GetVars/MustVars are the symbols to update).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/env/env.go`:
- Line 21: SetEnvKeyReplacer call is ineffective because Viper keys in this code
use underscores (e.g., auth_token, api_endpoint); either remove the
viper.SetEnvKeyReplacer(strings.NewReplacer(".", "_")) call or add a brief
comment above it clarifying it’s kept intentionally for future dot-to-underscore
mapping (e.g., supporting hierarchical keys like api.endpoint) so readers aren’t
confused; update the code around the viper.SetEnvKeyReplacer invocation (symbol:
viper.SetEnvKeyReplacer) accordingly.
- Line 16: Vars is currently created as an empty &Env{} and can be read before
Init(), so add a clear doc comment above the package-level variable Vars stating
that Init() must be called before any use; to harden further, change Vars to nil
by default and add a small accessor function (e.g., GetVars() or MustVars())
that checks Vars != nil and returns it or panics/errors with a clear message,
and update callers to use that accessor or ensure Init() is invoked early
(Init(), Vars, GetVars/MustVars are the symbols to update).
silv-io
left a comment
silv-io
left a comment
There was a problem hiding this comment.
LGTM! Just some ideas about how to handle cli env vars vs. container env vars
|
|
||
| // Init initializes environment variable configuration | ||
| func Init() { | ||
| viper.SetEnvPrefix("LOCALSTACK") |
There was a problem hiding this comment.
When I did this for the jam project I was super annoyed that you can't set two prefixes. I'd love to have a LSTK prefix for the CLI and a LOCALSTACK prefix for the container. You'd then need to handle both manually.
Then the LOCALSTACK prefix would just contain things that configure the container and are defined in here and the LSTK prefix would just be for things like the Keyring.
Maybe it's worth it to investigate how to do that best in a future iteration. Could be that it is possible but I just missed it. WDYT?
There was a problem hiding this comment.
I'm thinking all container-specific env vars should probably rather go in the config, wdyt? Otherwise it could get messy when having multiple containers.
But in any case for now I could rename our env vars to be prefixed with LSTK_ rather than LOCALSTACK_ since they are CLI-specific, makes sense?
There was a problem hiding this comment.
yeah let's do LSTK instead, faster to type as well :P
test/integration/env/env.go
Outdated
There was a problem hiding this comment.
nit: some maybe not necessary comments here. But I guess for hover mode in vim it's nice to see these
There was a problem hiding this comment.
That was way too much comments indeed. Removed.
| envWithout("LOCALSTACK_AUTH_TOKEN"), | ||
| "LOCALSTACK_API_ENDPOINT="+mockServer.URL, | ||
| ) | ||
| cmd.Env = env.Without(env.AuthToken).With(env.APIEndpoint, mockServer.URL) |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/env/env.go`:
- Around line 58-59: The With method on Environ currently appends a new
"key=value" string which can leave earlier entries for the same key and cause
non-deterministic behavior; modify Environ.With (the With function on type
Environ that accepts Key) to first remove any existing entries whose prefix
equals string(key)+"=" (filter the slice), then append the new
string(key)+"="+value so the environment deterministically contains only the
updated value for that key.
| func (e Environ) With(key Key, value string) Environ { | ||
| return append(e, string(key)+"="+value) |
There was a problem hiding this comment.
Ensure With overrides deterministically by removing existing keys first.
Appending without dedup can leave multiple entries for the same key; depending on the consumer, earlier entries might win, causing flaky behavior.
🔧 Suggested fix
func (e Environ) With(key Key, value string) Environ {
- return append(e, string(key)+"="+value)
+ e = e.Without(key)
+ return append(e, string(key)+"="+value)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/integration/env/env.go` around lines 58 - 59, The With method on Environ
currently appends a new "key=value" string which can leave earlier entries for
the same key and cause non-deterministic behavior; modify Environ.With (the With
function on type Environ that accepts Key) to first remove any existing entries
whose prefix equals string(key)+"=" (filter the slice), then append the new
string(key)+"="+value so the environment deterministically contains only the
updated value for that key.
2 participants
os.Getenv()calls throughout the codebase, eliminating raw string env var names.test/integration/envto do the same for integration tests.