Centralize env vars with Viper#39
Conversation
carole-lavillonniere
force-pushed
the
carole/drg-529
branch
2 times, most recently
from
4e14309 to
f090357
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
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. 🎉 📝 WalkthroughWalkthroughIntroduces a centralized environment configuration package Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (cmd/root)"
participant Env as "internal/env"
participant Config as "config.Init"
participant Components as "Auth/API/TokenStorage"
CLI->>Env: call Init()
Note right of Env: Viper loads LSTK_* envs\nsets defaults (api_endpoint, web_app_url)
Env-->>CLI: Vars populated
CLI->>Config: call Init()
Config-->>CLI: config ready
CLI->>Components: components read env.Vars (AuthToken, APIEndpoint, Keyring, WebAppURL)
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)
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
internal/env/env.go
Outdated
|
|
||
| // 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
| CI Key = "CI" | ||
| ) | ||
|
|
||
| // Get returns the value of the given environment variable. |
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.
carole-lavillonniere
force-pushed
the
carole/drg-529
branch
from
9968589 to
ab4e8ed
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/integration/start_test.go (1)
81-81: Potential duplicate env entry when chainingWith()calls.If
LOCALSTACK_AUTH_TOKENalready exists in the environment, chaining.With(env.AuthToken, "invalid-token")appends a second entry rather than replacing it. This relates to theEnviron.With()dedup issue noted intest/integration/env/env.go.Consider using
Withoutfirst to ensure deterministic behavior:♻️ Suggested change
- cmd.Env = env.With(env.AuthToken, "invalid-token").With(env.APIEndpoint, mockServer.URL) + cmd.Env = env.Without(env.AuthToken).With(env.AuthToken, "invalid-token").With(env.APIEndpoint, mockServer.URL)Alternatively, fixing
Environ.With()to deduplicate (as suggested in the env helper review) would resolve this globally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/start_test.go` at line 81, The test may append duplicate LOCALSTACK_AUTH_TOKEN entries by chaining With() on cmd.Env; change the setup to remove any existing auth token first by calling the environment helper's Without for env.AuthToken and then call With(env.AuthToken, "invalid-token") and With(env.APIEndpoint, mockServer.URL) so cmd.Env is deterministic; update the call site where cmd.Env is set (the line assigning cmd.Env) to use Without before With, or alternatively fix Environ.With() to deduplicate entries globally if you prefer a broader change.
🤖 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/env/env.go`:
- Around line 58-59: The With method of Environ is currently appending a
key=value pair which can leave duplicate keys and cause non-deterministic
behavior; update Environ.With (and use the Key type) to first filter/strip any
existing entries that start with string(key)+"=" from the receiver slice, then
append the new key=value entry and return that new slice so the environment has
at most one entry per key (preserve order by keeping other entries).
---
Nitpick comments:
In `@test/integration/start_test.go`:
- Line 81: The test may append duplicate LOCALSTACK_AUTH_TOKEN entries by
chaining With() on cmd.Env; change the setup to remove any existing auth token
first by calling the environment helper's Without for env.AuthToken and then
call With(env.AuthToken, "invalid-token") and With(env.APIEndpoint,
mockServer.URL) so cmd.Env is deterministic; update the call site where cmd.Env
is set (the line assigning cmd.Env) to use Without before With, or alternatively
fix Environ.With() to deduplicate entries globally if you prefer a broader
change.
2 participants
os.Getenv()calls throughout the codebase, eliminating raw string env var names.test/integration/envto do the same for integration tests.