-
Notifications
You must be signed in to change notification settings - Fork 763
Fix DockerComposeEnvironment duplicating image names in .env file #12310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: davidfowl <[email protected]>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12310Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12310" |
|
@captainsafia if we're not finishing the docker compose change then we need to get this in. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds functionality to the EnvFile class to support updating existing environment variable keys when onlyIfMissing is set to false, enabling the update of container image names and other environment variables in Docker .env files.
Key Changes
- Modified
EnvFile.Add()method to update existing keys whenonlyIfMissing=falseinstead of always skipping them - Added logic to locate and update the value of existing keys in the file while preserving file structure
- Added comprehensive test coverage for the new update behavior and edge cases
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Aspire.Hosting.Docker/EnvFile.cs | Refactored the Add method to support updating existing keys with new values when onlyIfMissing=false |
| tests/Aspire.Hosting.Docker.Tests/EnvFileTests.cs | Added new test file with six tests covering update behavior, duplicate prevention, and file loading scenarios |
| for (int i = 0; i < _lines.Count; i++) | ||
| { | ||
| var trimmed = _lines[i].TrimStart(); | ||
| if (!trimmed.StartsWith('#') && trimmed.Contains('=')) | ||
| { | ||
| var eqIndex = trimmed.IndexOf('='); | ||
| if (eqIndex > 0) | ||
| { | ||
| var lineKey = trimmed[..eqIndex].Trim(); | ||
| if (lineKey == key) | ||
| { | ||
| _lines[i] = value is not null ? $"{key}={value}" : $"{key}="; | ||
| return; | ||
| } | ||
| } | ||
| } | ||
| } |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The key-finding logic in lines 46-62 duplicates the parsing logic from the Load method (lines 19-31). Consider extracting this into a shared helper method to avoid code duplication and ensure consistent parsing behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored to extract the duplicate parsing logic into a shared TryParseKey helper method. This ensures consistent parsing behavior between the Load and Add methods. (f2a1504)
Co-authored-by: davidfowl <[email protected]>
|
/backport to release/13.0 |
|
Started backporting to release/13.0: https://github.com/dotnet/aspire/actions/runs/18961330038 |
Fix DockerComposeEnvironment Image Name Duplication
This PR fixes a regression where the DockerComposeEnvironment duplicates the image name in the .env file.
Root Cause
When
onlyIfMissing=falseis set for container image references (to always update the image name), theEnvFile.Addmethod didn't check if the key already exists and would add it again, causing duplicates.Changes Made
EnvFile.Addmethod to upsert (update or insert) whenonlyIfMissing=falseTechnical Details
The fix modifies
EnvFile.Add()to:onlyIfMissing=false, find and update the existing lineonlyIfMissing=true, skip adding (existing behavior)The latest commit addresses code review feedback by extracting the duplicate parsing logic from the
LoadandAddmethods into a sharedTryParseKeyhelper method, ensuring consistent parsing behavior.Files Changed
src/Aspire.Hosting.Docker/EnvFile.cs- Modified theAddmethod to handle upsert logic and extracted shared parsing logictests/Aspire.Hosting.Docker.Tests/EnvFileTests.cs- Added comprehensive unit tests for EnvFile behaviorTesting
Security Summary
No security vulnerabilities were detected in the changes.
Original prompt
Fixes #12309
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.