-
Notifications
You must be signed in to change notification settings - Fork 719
Fix .env files being overwritten by Docker Compose publisher #12656
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
base: main
Are you sure you want to change the base?
Conversation
When SaveKeysOnly is called, it now checks if keys already exist on disk with non-empty values and preserves those values instead of overwriting them. This ensures that user modifications to .env files are not lost during subsequent publish operations. Co-authored-by: captainsafia <[email protected]>
|
@copilot Make the following changes to the implementation:
|
- Store path on Load as private field - Remove path parameter from Save/SaveKeysOnly methods - Use existing values in Entries instead of reloading from disk - Change _entries to internal Entries property - Update PrepareAsync to initialize its own EnvFile - Update all test usages to call Save() without path parameter Co-authored-by: captainsafia <[email protected]>
Done in bd3a719. All changes have been implemented:
All 58 tests in Aspire.Hosting.Docker.Tests pass. |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12656Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12656" |
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 refactors the EnvFile class to track the file path internally and removes the need to pass the path as a parameter to Save() methods. This change addresses an issue where user-modified values in environment files were being overwritten during publish operations.
Key changes:
- The
EnvFileclass now stores the file path internally (set duringLoad()) Save()methods no longer require a path parameter- The
SaveKeysOnly()method now preserves existing non-empty values instead of always writing empty values SharedEnvFileproperty is removed from the publishing workflow in favor of loading fresh instances
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting.Docker/EnvFile.cs | Refactored to store path internally, changed Save() to parameterless, and modified SaveKeysOnly() to preserve existing values |
| src/Aspire.Hosting.Docker/DockerComposePublishingContext.cs | Updated to call Save() without path parameter |
| src/Aspire.Hosting.Docker/DockerComposeEnvironmentResource.cs | Changed to load a fresh EnvFile instance in PrepareAsync() instead of reusing SharedEnvFile |
| tests/Aspire.Hosting.Docker.Tests/EnvFileTests.cs | Updated all test calls to use parameterless Save() |
| tests/Aspire.Hosting.Docker.Tests/Snapshots/DockerComposePublisherTests.DockerComposeDoesNotOverwriteEnvFileOnPublish#01.verified.env | Updated snapshot showing preserved user-modified value |
| tests/Aspire.Hosting.Docker.Tests/Snapshots/DockerComposePublisherTests.DockerComposeAppendsNewKeysToEnvFileOnPublish#01.verified.env | Updated snapshot showing preserved user-modified value when new keys are appended |
src/Aspire.Hosting.Docker/EnvFile.cs
Outdated
| { | ||
| lines.Add($"{entry.Key}="); | ||
| } | ||
|
|
Copilot
AI
Nov 4, 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.
Trailing whitespace on line 134. The blank line before lines.Add(string.Empty); contains only whitespace characters, which violates coding standards. Remove the trailing whitespace.
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.
Fixed in eb5371f.
| private readonly SortedDictionary<string, EnvEntry> _entries = []; | ||
| private string? _path; | ||
|
|
||
| internal SortedDictionary<string, EnvEntry> Entries { get; } = []; |
Copilot
AI
Nov 4, 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 Entries property is marked as internal but was previously a private field _entries. Exposing the internal dictionary directly allows external code within the assembly to modify it without going through the controlled Add() method, potentially bypassing validation logic. Consider keeping this as a private field or exposing it as IReadOnlyDictionary<string, EnvEntry> to prevent uncontrolled mutations.
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.
Is Entries used outside of this class?
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.
No, Entries is only used within the EnvFile class itself. The grep results show it's only accessed internally in EnvFile.cs at line 42 where it assigns values during the Load operation.
|
@copilot When saving a envfile with keys and values, check if a file with that name already exists and emit an info log indicating it will be overwritten if it does. |
- Add ILogger parameter to EnvFile.Load() and constructor - Log at Information level when saving with values overwrites existing file - Fix trailing whitespace on line 134 - Update callers to pass logger from context - All tests pass Co-authored-by: captainsafia <[email protected]>
Done in eb5371f. When saving an environment file with keys and values, the code now logs at Information level if a file with that name already exists and will be overwritten: if (File.Exists(_path))
{
_logger?.LogInformation("Environment file '{EnvFilePath}' already exists and will be overwritten", _path);
}The logger is now passed through the |
|
@copilot Add tests to |
- Add EnvFile.Create() method to create new env file without loading existing content - Update PrepareAsync to use Create() instead of Load() to avoid loading old entries - Add PrepareStep_OverwritesExistingEnvFileAndLogsWarning test - Add PrepareStep_OverwritesExistingEnvFileWithCustomEnvironmentName test - Both tests verify file is overwritten and log message is emitted - All 60 tests pass Co-authored-by: captainsafia <[email protected]>
Done in 2df4bf4. Added two tests to validate
Both tests:
Also fixed All 60 tests pass. |
- Remove SharedEnvFile property from DockerComposeEnvironmentResource - Remove SharedEnvFile usage from DockerComposePublishingContext - EnvFile is now loaded fresh on each publish operation - All 60 tests pass Co-authored-by: captainsafia <[email protected]>
Fixes an issue where user-modified values in
.envfiles were being overwritten during Docker Compose publish operations.Changes Made
Core Fix
EnvFile.SaveKeysOnly()to preserve existing non-empty values from disk instead of always writing empty valuesEnvFileto store the file path internally, eliminating the need to pass path parameters toSave()methods_entriesfrom a private field to an internalEntriesproperty for better accessibility within the assemblyLogging Enhancement
ILoggerparameter toEnvFile.Load()andCreate()methods to enable logging throughout the workflowPrepareAsync Improvement
EnvFile.Create()method to create new env files without loading existing contentPrepareAsyncto useCreate()instead ofLoad()to ensure clean environment files during the prepare stepCleanup
SharedEnvFileproperty as EnvFile is now loaded fresh on each operationTesting
PrepareStep_OverwritesExistingEnvFileAndLogsWarningandPrepareStep_OverwritesExistingEnvFileWithCustomEnvironmentName) to validate env file overwriting behavior and log message emissionPARAM1=changedare preservedTest Results
Fixes #12650
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.