Skip to content

Comments

avoid printing env vars conflict warnings if there is no conflict#1807

Merged
lionello merged 2 commits intomainfrom
jordan/avoid-immaterial-env-warning
Jan 19, 2026
Merged

avoid printing env vars conflict warnings if there is no conflict#1807
lionello merged 2 commits intomainfrom
jordan/avoid-immaterial-env-warning

Conversation

@jordanstephens
Copy link
Member

@jordanstephens jordanstephens commented Jan 19, 2026

Description

To reduce noise, lets avoid printing this warning if the value in the environment and the value in the stack are the same

Linked Issues

Checklist

  • I have performed a self-review of my code
  • I have added appropriate tests
  • I have updated the Defang CLI docs and/or README to reflect my changes, if necessary

Summary by CodeRabbit

  • Bug Fixes
    • Environment values are now preserved and applied using their actual values rather than as simple presence flags.
    • Conflict detection now warns only when environment-provided values differ from stack values, reducing false positives.
    • Improved application logic for when environment variables are set or overridden, with clearer warning text reflecting value sources.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 19, 2026

📝 Walkthrough

Walkthrough

The LoadStackEnv function in stacks.go now preserves environment variable values by switching from map[string]bool to map[string]string, using strings.Cut to extract key/value pairs, updating conflict detection to compare values, and adjusting when values are applied and warnings emitted.

Changes

Cohort / File(s) Summary
Environment Variable Value Handling
src/pkg/stacks/stacks.go
LoadStackEnv changed from map[string]bool to map[string]string; uses strings.Cut to extract key/value; conflict check now compares envValue != value; apply logic sets values when absent or when overload allowed; warning text updated to reference the environment value.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • avoid double stack loading #1800 — Adjusts caller-side usage of LoadStackEnv and moves/removes wrappers, directly related to the function signature/semantics change in this PR.

Poem

🐰 I hopped through envs with scissors bright,
I cut the pairs and kept each light,
No longer true/false in the air,
Values settle, tidy and fair,
Hooray — keys and values hold tight!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly summarizes the main change: suppressing environment variable conflict warnings when there is no actual conflict, which aligns with the core objective to reduce noise from false-positive warnings.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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"
level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/pkg/stacks/stacks.go`:
- Around line 229-234: The warning is emitted even when the env var doesn't
exist because currentEnv[key] returns "" for missing keys; change the logic in
the loop over paramsMap to perform the existence check first (use the lookup
form like "val, ok := currentEnv[key]") and only call term.Warnf when ok is true
and val != value and !overload; keep the existing branch that sets the env via
os.Setenv when the key is missing or overload is true so behavior remains the
same.

@lionello lionello merged commit 882ecee into main Jan 19, 2026
14 checks passed
@lionello lionello deleted the jordan/avoid-immaterial-env-warning branch January 19, 2026 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants