Add IFileSystemService abstraction for simplified temp file management #13282
Conversation
| /// Deletes the temporary file. When created via <see cref="ITempFileSystemService.CreateTempFile(string, string)"/>, | ||
| /// also deletes the parent directory. |
There was a problem hiding this comment.
Fixed in 1f75f9a. The issue was invalid XML doc syntax - <see cref="ITempFileSystemService.CreateTempFile(string, string)"/> doesn't work for referencing overloaded methods. Changed to descriptive text: "When created with a specific file name in a temporary directory, also deletes the parent directory." Also fixed an unrelated backslash escape error on line 47.
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public TempFile CreateTempFile(string prefix, string fileName) |
There was a problem hiding this comment.
@copilot We just make this CreateTempFile that takes no arguments. Also remove the other overload.
There was a problem hiding this comment.
Simplified in ea082bc. Removed both overloads and replaced with single CreateTempFile() method that takes no parameters. Updated AspireStore.cs to use the simplified API.
|
@copilot Check failure on line 450 in tests\Aspire.Hosting.Tests\PublishAsDockerfileTests.cs @azure-pipelines tests\Aspire.Hosting.Tests\PublishAsDockerfileTests.cs(450,20): error ASPIREFILESYSTEM001: (NETCORE_ENGINEERING_TELEMETRY=Build) 'Aspire.Hosting.TempDirectory' is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. (https://aka.ms/aspire/diagnostics/ASPIREFILESYSTEM001) @azure-pipelines tests\Aspire.Hosting.Tests\AspireStoreTests.cs(126,47): error CS7036: (NETCORE_ENGINEERING_TELEMETRY=Build) There is no argument given that corresponds to the required parameter 'directoryService' of 'AspireStore.AspireStore(string, IFileSystemService)' @azure-pipelines tests\Aspire.Hosting.Tests\Dcp\DcpHostNotificationTests.cs(27,29): error CS7036: (NETCORE_ENGINEERING_TELEMETRY=Build) There is no argument given that corresponds to the required parameter 'directoryService' of 'Locations.Locations(IFileSystemService)' @azure-pipelines tests\Aspire.Hosting.Tests\Dcp\DcpHostNotificationTests.cs(67,29): error CS7036: (NETCORE_ENGINEERING_TELEMETRY=Build) There is no argument given that corresponds to the required parameter 'directoryService' of 'Locations.Locations(IFileSystemService)' @azure-pipelines tests\Aspire.Hosting.Tests\Dcp\DcpHostNotificationTests.cs(116,29): error CS7036: (NETCORE_ENGINEERING_TELEMETRY=Build) There is no argument given that corresponds to the required parameter 'directoryService' of 'Locations.Locations(IFileSystemService)' |
Fixed in 9358941. Addressed all build failures:
|
|
@copilot Now update all of the high priorty items in IMPLEMENTATION_STATUS.md and keep it up to date with progress. |
Completed in 624e43d. Updated all high priority files:
IMPLEMENTATION_STATUS.md updated to reflect completion of high priority items. Remaining: Azure integration files, MAUI files, MySQL, testing infrastructure, and documentation. |
|
@copilot We should make CreateTempFile take an optional file name |
Implemented in 109ba23. |
|
@copilot Rename the shared helper TempDirectory to TestTempDirectory |
Renamed in 62749e8. The shared helper class in |
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13282Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13282" |
Co-authored-by: davidfowl <[email protected]> Update DistributedApplicationBuilder to register and expose IFileSystemService Co-authored-by: davidfowl <[email protected]> Update AspireStore and Locations to use IFileSystemService Co-authored-by: davidfowl <[email protected]> Add implementation status document with remaining work Co-authored-by: davidfowl <[email protected]> Fix invalid XML doc comment syntax for TempFile.Dispose Co-authored-by: davidfowl <[email protected]> Simplify CreateTempFile API to take no parameters Co-authored-by: davidfowl <[email protected]> Fix test compilation errors - add experimental warnings and fix constructor calls Co-authored-by: davidfowl <[email protected]> Update high priority files to use IFileSystemService Co-authored-by: davidfowl <[email protected]> Add optional fileName parameter to CreateTempFile Co-authored-by: davidfowl <[email protected]> Rename shared TempDirectory helper class to TestTempDirectory Co-authored-by: davidfowl <[email protected]> Refactor file handling to use IFileSystemService for temporary file management in Maui environment annotations and MySql builder extensions Update implementation status to reflect IFileSystemService integration across MySQL and MAUI components
davidfowl
force-pushed
the
copilot/adjustments-from-pr-feedback
branch
from
c9322e9 to
8f3fcdf
Compare
- Updated multiple test files across MySql, PostgreSQL, Python, and Yarp tests to replace instances of TempDirectory with TestTempDirectory for improved test isolation and resource management. - Ensured consistent usage of TestTempDirectory in various test scenarios, including configuration file generation and environment variable handling.
…istributedApplicationTestingBuilder
…ntimeConfig method
davidfowl
requested review from
ReubenBond,
captainsafia,
eerhardt,
jfversluis and
mitchdenny
as code owners
There was a problem hiding this comment.
Pull request overview
This PR implements the IFileSystemService abstraction to provide centralized temporary file and directory management across Aspire, replacing direct calls to Path.GetTempFileName() and Directory.CreateTempSubdirectory(). The implementation is marked experimental and enables better testability and consistent temp file handling.
Key Changes:
- New
IFileSystemServiceAPI withCreateTempFile()andCreateTempSubdirectory()methods - Integration into
DistributedApplicationBuilderwith DI registration - Migration of core components (PipelineOutputService, DashboardEventHandlers, Locations, AspireStore, etc.)
- Refactored
UserSecretsManagerFactoryto acceptIFileSystemServiceas dependency - Test renaming from
TempDirectorytoTestTempDirectoryto avoid naming conflicts - Comprehensive test coverage in new
FileSystemServiceTests.cs
Reviewed changes
Copilot reviewed 60 out of 60 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting/Utils/IFileSystemService.cs |
New experimental API for file system operations with temp file/directory management |
src/Aspire.Hosting/Utils/FileSystemService.cs |
Default implementation with proper cleanup semantics |
src/Aspire.Hosting/IDistributedApplicationBuilder.cs |
Added FileSystemService and UserSecretsManager properties (with minor doc issues) |
src/Aspire.Hosting/DistributedApplicationBuilder.cs |
Service registration and initialization |
src/Aspire.Hosting/Pipelines/PipelineOutputService.cs |
Migrated from IConfiguration-based temp dir to IFileSystemService |
src/Aspire.Hosting/Dashboard/DashboardEventHandlers.cs |
Updated to use IFileSystemService for runtime config files |
src/Aspire.Hosting/Dcp/Locations.cs |
Refactored to use IFileSystemService for DCP session directories |
src/Aspire.Hosting/ApplicationModel/AspireStore.cs |
Updated constructor to accept IFileSystemService |
src/Aspire.Hosting/ApplicationModel/ProjectResource.cs |
Uses IFileSystemService for Dockerfile generation |
src/Aspire.Hosting/ContainerResourceBuilderExtensions.cs |
Uses builder.ApplicationBuilder.FileSystemService for temp files |
src/Aspire.Hosting/UserSecrets/* |
Refactored to use IFileSystemService, removed singleton pattern |
src/Aspire.Hosting/ParameterResourceBuilderExtensions.cs |
Updated to use builder.UserSecretsManager instead of assembly-based approach |
src/Aspire.Hosting.Testing/DistributedApplicationTestingBuilder.cs |
Exposed new properties for testing scenarios |
src/Aspire.Hosting.MySql/MySqlBuilderExtensions.cs |
Updated PhpMyAdmin config file creation |
src/Aspire.Hosting.Maui/Utilities/* |
Updated environment annotation subscribers to use IFileSystemService |
src/Aspire.Hosting.Azure/* |
Updated BicepProvisioner and AzurePublishingContext for temp bicep files |
tests/Shared/TempDirectory.cs |
Renamed to TestTempDirectory to avoid naming conflict |
tests/**/FileSystemServiceTests.cs |
New comprehensive test coverage for IFileSystemService |
| All test files | Updated to use TestTempDirectory, added experimental pragmas, fixed constructor calls |
| /// </para> | ||
| /// </remarks> | ||
| [Experimental("ASPIREUSERSECRETS001", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] | ||
| public IUserSecretsManager UserSecretsManager => throw new NotImplementedException(); |
There was a problem hiding this comment.
This property has a default interface implementation that throws NotImplementedException. While this is intended to force implementations to override it, this could be confusing and may break consumers who try to call it on the interface directly. Consider documenting this behavior clearly or using a different pattern.
According to the custom guideline for XML Documentation Standards, default interface implementations should be clearly documented with remarks about their behavior.
|
|
||
| _outputPath = options.Value.OutputPath is not null ? Path.GetFullPath(options.Value.OutputPath) : null; | ||
| _tempDirectory = new Lazy<string>(() => CreateTempDirectory(configuration)); | ||
| _tempDirectory = directoryService.TempDirectory.CreateTempSubdirectory("aspire-pipelines").Path; |
There was a problem hiding this comment.
[nitpick] The temp directory created here is stored but never disposed, which means it will persist until the process ends. This is likely intentional for the lifetime of PipelineOutputService, but the pattern doesn't match the dispose semantics described in the API documentation.
Consider documenting this pattern more explicitly, or ensuring the temp directory is cleaned up when the service is disposed (if it implements IDisposable).
There was a problem hiding this comment.
Lets keep the lazy implementation.
| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// The <see cref="FileSystemService"/> provides a centralized way to manage temporary files and directories |
There was a problem hiding this comment.
The XML documentation refers to <see cref="FileSystemService"/> but this is an implementation class, not the property type. The property returns IFileSystemService. Change the reference to match the actual return type.
Suggested fix:
/// <para>
/// The <see cref="IFileSystemService"/> provides a centralized way to manage temporary files and directories| /// </summary> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// The <see cref="UserSecretsManager"/> provides a centralized way to manage user secrets |
There was a problem hiding this comment.
The XML documentation refers to <see cref="UserSecretsManager"/> but this is an implementation class, not the interface. The property returns IUserSecretsManager. Change the reference to match the actual return type.
Suggested fix:
/// <para>
/// The <see cref="IUserSecretsManager"/> provides a centralized way to manage user secrets| /// </para> | ||
| /// </remarks> | ||
| [Experimental("ASPIREFILESYSTEM001", UrlFormat = "https://aka.ms/aspire/diagnostics/{0}")] | ||
| public IFileSystemService FileSystemService => throw new NotImplementedException(); |
There was a problem hiding this comment.
This property has a default interface implementation that throws NotImplementedException. While this is intended to force implementations to override it, this could be confusing and may break consumers who try to call it on the interface directly. Consider documenting this behavior clearly or using a different pattern.
According to the custom guideline for XML Documentation Standards, default interface implementations should be clearly documented with remarks about their behavior.
|
This is good overall I think. The one thing we should think about adding is a list of allocated temporary files/directories. When a file is disposed it should remove itself from that list - but the idea is that when the apphost is shutting down we can quickly spin over the list and delete any files. Probably also want an environment varaible that the file system service uses to preserve files for debugging scenarios. The file system service also needs verbose logging when allocating files/directories as well. |
Implementation of PR #13244 with Feedback Adjustments
This PR implements the IFileSystemService abstraction for temporary file/directory management across Aspire, addressing feedback from the original PR #13244.
✅ Completed Work
Core API Implementation
IFileSystemServiceandFileSystemServicewith simplified experimental APICreateTempFile(string? fileName = null)- optional fileName parameter for named temp filesCreateTempSubdirectory(prefix?)for creating temp directoriesFileSystemServiceproperty toIDistributedApplicationBuilderIFileSystemServiceinDistributedApplicationBuilderAspireStoreto use simplifiedIFileSystemService.CreateTempFile()Locations(DCP) to useIFileSystemServiceTempDirectorytoTestTempDirectoryto avoid naming conflictHigh Priority Core Files - Complete
📋 Remaining Work (See IMPLEMENTATION_STATUS.md)
Integration Files:
Testing:
Documentation:
🎯 Key Improvements Over Original PR
CreateTempFile(string? fileName = null)supports both random and named temp files📝 Notes
.Pathand not disposing is intentional - many temp files persist for app lifetimeCreateTempFile()creates a simple temp file usingPath.GetTempFileName()CreateTempFile("filename.ext")creates a temp subdirectory with the named file inside, cleans up both on disposeCreateTempSubdirectory(prefix)creates a temp directory that can be disposed for cleanup[Experimental("ASPIREFILESYSTEM001")]TempDirectorytoTestTempDirectoryto avoid conflict withAspire.Hosting.TempDirectoryOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.