warn about conflicting provider env var#1825
Conversation
📝 WalkthroughWalkthroughThe PR refines the warning message logic in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)level=warning msg="[linters_context] running gomodguard failed: unable to read module file go.mod: current working directory must have a go.mod file: if you are not using go modules it is suggested to disable this linter" 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: 1
🤖 Fix all issues with AI agents
In `@src/pkg/stacks/stacks.go`:
- Around line 229-237: The warning for DEFANG_PROVIDER contradicts behavior
because when overload is false we currently warn that the stack value will be
used but do not call os.Setenv; change the logic so DEFANG_PROVIDER is always
written into the environment regardless of overload. Specifically, in the block
that checks currentEnv[key], ensure that if key == "DEFANG_PROVIDER" you call
os.Setenv(key, value) (or treat it as if (_, ok := currentEnv[key]; !ok ||
overload) is true) so the stack value overrides the existing env; adjust the
term.Warnf message if needed to reflect that DEFANG_PROVIDER is being forced,
and keep references to currentEnv, overload, os.Setenv, term.Warnf, and the
"DEFANG_PROVIDER" constant to locate the code.
| if envValue, ok := currentEnv[key]; ok && envValue != value && !overload { | ||
| term.Warnf("The variable %q is set in both the stack and the environment. The value from the environment will be used.\n", key) | ||
| if key == "DEFANG_PROVIDER" { | ||
| term.Warnf("The variable DEFANG_PROVIDER is set to %q in the environment, but the stack specifies %q. The value from the stack will be used.\n", envValue, value) | ||
| } else { | ||
| term.Warnf("The variable %q is set to %q in the environment, but the stack specifies %q. The value from the environment will be used.\n", key, envValue, value) | ||
| } | ||
| } | ||
| if _, ok := currentEnv[key]; !ok || overload { | ||
| err := os.Setenv(key, value) |
There was a problem hiding this comment.
Warning contradicts behavior for DEFANG_PROVIDER; env is still left untouched.
When overload is false and DEFANG_PROVIDER is already set, the warning says the stack value will be used, but the code path does not call os.Setenv, so downstream code reading os.Getenv("DEFANG_PROVIDER") will still see the environment value. This is exactly the mismatch the PR aims to fix. Consider forcing an override for DEFANG_PROVIDER even when overload is false.
🔧 Proposed fix
- if _, ok := currentEnv[key]; !ok || overload {
+ shouldOverride := overload || key == "DEFANG_PROVIDER"
+ if _, ok := currentEnv[key]; !ok || shouldOverride {
err := os.Setenv(key, value)
if err != nil {
return fmt.Errorf("could not set env var %q: %w", key, err)
}
}🤖 Prompt for AI Agents
In `@src/pkg/stacks/stacks.go` around lines 229 - 237, The warning for
DEFANG_PROVIDER contradicts behavior because when overload is false we currently
warn that the stack value will be used but do not call os.Setenv; change the
logic so DEFANG_PROVIDER is always written into the environment regardless of
overload. Specifically, in the block that checks currentEnv[key], ensure that if
key == "DEFANG_PROVIDER" you call os.Setenv(key, value) (or treat it as if (_,
ok := currentEnv[key]; !ok || overload) is true) so the stack value overrides
the existing env; adjust the term.Warnf message if needed to reflect that
DEFANG_PROVIDER is being forced, and keep references to currentEnv, overload,
os.Setenv, term.Warnf, and the "DEFANG_PROVIDER" constant to locate the code.
|
After thinking about this more, I'm closing this PR and will instead fix this in #1820. See |
1 participant
Description
Lio reported:
So this warning is wrong. We actually ignore the environment variable in this case.
DEFANG_PROVIDERis the only stack variable which can not be overridden by the environment, so this PR updates this warning logic to handleDEFANG_PROVIDERspecifically.Linked Issues
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.