Add AppHostEnvironment as public sealed class #13085
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13085Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13085" |
davidfowl
requested review from
ReubenBond,
captainsafia,
eerhardt and
mitchdenny
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR introduces AppHostEnvironment as a public sealed class to centralize access to AppHost configuration values, replacing direct IConfiguration["AppHost:*"] access throughout the codebase with strongly-typed properties.
Key Changes:
- Introduces
AppHostEnvironmentclass with properties for project name, directories, hashes, API keys, and other AppHost-specific configuration - Updates
IDistributedApplicationBuilderto exposeAppHostEnvironmentproperty - Refactors 25+ production files to use strongly-typed properties instead of configuration string keys
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting/AppHostEnvironment.cs | New public sealed class centralizing AppHost configuration access |
| src/Aspire.Hosting/IDistributedApplicationBuilder.cs | Adds AppHostEnvironment property with default implementation |
| src/Aspire.Hosting/DistributedApplicationBuilder.cs | Initializes AppHostEnvironment and registers in DI |
| src/Aspire.Hosting/VolumeNameGenerator.cs | Uses ProjectName and DefaultHash instead of configuration keys |
| src/Aspire.Hosting/VersionChecking/VersionCheckService.cs | Uses ProjectDirectory from AppHostEnvironment |
| src/Aspire.Hosting/ResourceLoggerForwarderService.cs | Uses ProjectName for logger category names |
| src/Aspire.Hosting/ProjectResourceBuilderExtensions.cs | Uses DefaultLaunchProfileName property |
| src/Aspire.Hosting/Pipelines/Internal/FileDeploymentStateManager.cs | Uses FullPathHash for deployment state |
| src/Aspire.Hosting/ParameterResourceBuilderExtensions.cs | Uses ProjectName for user secrets |
| src/Aspire.Hosting/OtlpConfigurationExtensions.cs | Uses OtlpApiKey property |
| src/Aspire.Hosting/DistributedApplicationLifecycle.cs | Uses ProjectDirectory for logging |
| src/Aspire.Hosting/Dcp/DcpNameGenerator.cs | Uses DefaultHash for name generation |
| src/Aspire.Hosting/Dcp/DcpExecutor.cs | Uses ContainerHostname property |
| src/Aspire.Hosting/Dashboard/DashboardService.cs | Uses DashboardApplicationName property |
| src/Aspire.Hosting/Dashboard/DashboardOptions.cs | Uses OtlpApiKey property |
| src/Aspire.Hosting/Dashboard/DashboardEventHandlers.cs | Uses ResourceServiceAuthMode and ResourceServiceApiKey |
| src/Aspire.Hosting.Testing/DistributedApplicationTestingBuilder.cs | Exposes AppHostEnvironment property |
| src/Aspire.Hosting.Azure/Provisioning/Internal/BaseProvisioningContextProvider.cs | Uses AppHostEnvironment instead of IHostEnvironment |
| src/Aspire.Hosting.Azure/Provisioning/Internal/RunModeProvisioningContextProvider.cs | Uses ProjectName for resource group names |
| src/Aspire.Hosting.Azure/Provisioning/Internal/PublishModeProvisioningContextProvider.cs | Uses ProjectName for resource group names |
| src/Aspire.Hosting.Azure/AzureEnvironmentResourceExtensions.cs | Uses ProjectNameHash for stable naming |
| src/Aspire.Hosting.Azure.Functions/AzureFunctionsProjectResourceExtensions.cs | Uses ProjectNameHash for storage names |
| tests/Aspire.Hosting.Tests/Utils/TestAppHostEnvironment.cs | New test helper factory for creating AppHostEnvironment instances |
| tests/Aspire.Hosting.Tests/VersionChecking/VersionCheckServiceTests.cs | Updated to use TestAppHostEnvironment.Create |
| tests/Aspire.Hosting.Tests/Publishing/DeploymentStateManagerTests.cs | Updated to construct AppHostEnvironment |
| tests/Aspire.Hosting.Tests/Dcp/DcpExecutorTests.cs | Updated to construct AppHostEnvironment |
| tests/Aspire.Hosting.Tests/Dashboard/DashboardServiceTests.cs | Updated to use TestAppHostEnvironment.Create |
| tests/Aspire.Hosting.Tests/Dashboard/DashboardLifecycleHookTests.cs | Updated to use TestAppHostEnvironment.Create |
| tests/Aspire.Hosting.Testing.Tests/ResourceLoggerForwarderServiceTests.cs | Updated to construct AppHostEnvironment |
| tests/Aspire.Hosting.Azure.Tests/ProvisioningTestHelpers.cs | Added CreateAppHostEnvironment helper method |
| tests/Aspire.Hosting.Azure.Tests/ProvisioningContextProviderTests.cs | Updated to use CreateAppHostEnvironment |
| tests/Aspire.Hosting.Azure.Tests/AzureDeployerTests.cs | Updated to use CreateAppHostEnvironment |
| /// Gets the directory of the project where the app host is located. | ||
| /// </summary> | ||
| public string ProjectDirectory => _configuration["AppHost:Directory"]!; |
There was a problem hiding this comment.
The null-forgiving operator (!) is used here, but ProjectDirectory can legitimately be null if the configuration value is not set. Consider making this property nullable (string?) to properly represent the possibility of a missing value, or document that this value is guaranteed to be present.
| /// Gets the directory of the project where the app host is located. | |
| /// </summary> | |
| public string ProjectDirectory => _configuration["AppHost:Directory"]!; | |
| /// Gets the directory of the project where the app host is located, or <c>null</c> if not set. | |
| /// </summary> | |
| public string? ProjectDirectory => _configuration["AppHost:Directory"]; |
There was a problem hiding this comment.
Added documentation clarifying that ProjectDirectory is guaranteed to be set by the build system at runtime. The null-forgiving operator is correct for this property. (4a76bff)
| /// Gets the SHA256 hash of the app host. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// For backward compatibility, this uses mode-dependent logic: | ||
| /// - Publish mode: ProjectNameSha (stable across paths) | ||
| /// - Run mode: FullPathHash (disambiguates by path) | ||
| /// </remarks> | ||
| public string DefaultHash => _configuration["AppHost:Sha256"]!; |
There was a problem hiding this comment.
The null-forgiving operator (!) is used here, but DefaultHash can legitimately be null if the configuration value is not set. Consider making this property nullable (string?) to properly represent the possibility of a missing value, or document that this value is guaranteed to be present.
| /// Gets the SHA256 hash of the app host. | |
| /// </summary> | |
| /// <remarks> | |
| /// For backward compatibility, this uses mode-dependent logic: | |
| /// - Publish mode: ProjectNameSha (stable across paths) | |
| /// - Run mode: FullPathHash (disambiguates by path) | |
| /// </remarks> | |
| public string DefaultHash => _configuration["AppHost:Sha256"]!; | |
| /// Gets the SHA256 hash of the app host, if available. | |
| /// </summary> | |
| /// <remarks> | |
| /// For backward compatibility, this uses mode-dependent logic: | |
| /// <list type="bullet"> | |
| /// <item>Publish mode: ProjectNameSha (stable across paths)</item> | |
| /// <item>Run mode: FullPathHash (disambiguates by path)</item> | |
| /// </list> | |
| /// This property may be <c>null</c> if the configuration value <c>AppHost:Sha256</c> is not set. | |
| /// </remarks> | |
| public string? DefaultHash => _configuration["AppHost:Sha256"]; |
There was a problem hiding this comment.
Added documentation clarifying that DefaultHash is guaranteed to be set by the build system at runtime. The null-forgiving operator is correct for this property. (4a76bff)
| /// This is the project name used in multiple places throughout the application, | ||
| /// including for generating resource names and configuration keys. | ||
| /// </remarks> | ||
| public string ProjectName => _configuration["AppHost:DashboardApplicationName"] ?? _hostEnvironment.ApplicationName; |
There was a problem hiding this comment.
The ProjectName and DashboardApplicationName properties (lines 35 and 50) have identical implementations. This duplication suggests they serve different semantic purposes but are currently implemented the same way. Consider either consolidating them if they're truly the same, or adding distinct remarks explaining when to use each property.
There was a problem hiding this comment.
Added documentation explaining that DashboardApplicationName is provided separately for semantic clarity when specifically referring to the dashboard display name, though it returns the same value as ProjectName. (4a76bff)
There was a problem hiding this comment.
@davidfowl - I agree with the first Copilot that it is confusing to have 2 properties that are the same thing.
There was a problem hiding this comment.
@copilot lets unify on ProjectName then, this should also trim the "apphost" suffix from the name if it exists.
There was a problem hiding this comment.
Unified on ProjectName by removing DashboardApplicationName. The ProjectName property now automatically trims the ".AppHost" suffix (case-insensitive) if present. Also simplified DashboardService to use ProjectName directly and removed the now-unused regex logic. (b66498b)
| #pragma warning disable IDE0005 // IConfiguration is used in this file | ||
|
|
There was a problem hiding this comment.
The pragma disable comment 'IConfiguration is used in this file' is misleading. IConfiguration IS imported and used in this file (line 36 and field _configuration on line 65). This pragma should either be removed if the using is necessary, or the comment should be updated to explain why the warning is being suppressed despite the using statement being present.
| #pragma warning disable IDE0005 // IConfiguration is used in this file |
There was a problem hiding this comment.
Updated the pragma comment to be more accurate. The IConfiguration using directive is necessary as the field _configuration is actively used throughout the file. (4a76bff)
| /// <summary> | ||
| /// Creates a test AppHost environment. | ||
| /// </summary> | ||
| public static AppHostEnvironment CreateAppHostEnvironment() |
There was a problem hiding this comment.
According to the XML documentation standards, internal methods should have minimal documentation. The <summary> tag here is sufficient, but if there were more verbose documentation (like <remarks> or <example>), it should be removed for internal APIs.
|
@copilot Look at the test failures on this PR, they are related to: System.InvalidOperationException : No service for type 'Aspire.Hosting.AppHostEnvironment' has been registered. at Microsoft.Extensions.DependencyInjection.ServiceProviderServiceExtensions.GetRequiredService(IServiceProvider provider, Type serviceType) |
Fixed the test failure by changing |
|
@copilot That's the wrong fix, the right fix is to make sure that |
Create AppHostEnvironment as public class and update core hosting files Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> Update Azure provisioning and test files to use AppHostEnvironment Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> Address code review feedback: Add documentation and fix test failure - Added documentation explaining that configuration values are guaranteed by build system - Added semantic distinction between ProjectName and DashboardApplicationName - Fixed test failure by making AppHostEnvironment retrieval safe with fallback to IConfiguration - Updated pragma comment to be more accurate Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> Fix test infrastructure: Register AppHostEnvironment in TestServiceProvider - Reverted fallback approach in OtlpConfigurationExtensions (now uses GetRequiredService) - Added AppHostEnvironment registration in TestServiceProvider constructor - Tests now properly have AppHostEnvironment available in their service provider Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> Expose Configuration and HostEnvironment as public properties Added public Configuration and HostEnvironment properties to AppHostEnvironment class so consumers can access the underlying services directly without needing to inject multiple DI services. Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> Unify on ProjectName and remove DashboardApplicationName - Removed DashboardApplicationName property (unified on ProjectName) - Updated ProjectName to automatically trim ".AppHost" suffix (case-insensitive) - Simplified DashboardService to use ProjectName directly - Removed unused ApplicationNameRegex and System.Text.RegularExpressions using Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> Rename ProjectName to ApplicationName for consistency across the codebase
…plicationName - Add DisplayName property to AppHostEnvironment that returns a display-friendly application name (strips .AppHost suffix for traditional app hosts, uses directory name for single-file app hosts) - Rename DashboardApplicationName to SingleFileAppHostName in DistributedApplicationOptions to better reflect its purpose - Update DashboardService to use DisplayName for the dashboard display - ApplicationName now returns the full name from IHostEnvironment without modification 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
davidfowl
force-pushed
the
copilot/make-class-public-instead-of-interface
branch
from
b66498b to
0f2d970
Compare
| /// </summary> | ||
| /// <remarks> | ||
| /// For backward compatibility, this uses mode-dependent logic: | ||
| /// - Publish mode: ProjectNameSha (stable across paths) |
There was a problem hiding this comment.
| /// - Publish mode: ProjectNameSha (stable across paths) | |
| /// - Publish mode: ProjectNameHash (stable across paths) |
| /// Gets the SHA256 hash of the app host. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// For backward compatibility, this uses mode-dependent logic: |
There was a problem hiding this comment.
What does "For backward compatibility" mean here?
There was a problem hiding this comment.
Do we really need this one? It is only used in 2 places internally. If this is for "backward compat", maybe it shouldn't be public?
Description
This PR implements
AppHostEnvironmentas a public sealed class to centralize AppHost configuration access, based on PR #13018 but with key modifications per requirements.Successfully implemented
AppHostEnvironmentas a public sealed class (not an interface) to centralize AppHost configuration access.Key Changes:
AppHostEnvironmentsealed class that can be constructed directlyDashboardApplicationNameproperty to eliminate confusion, unified onProjectNamewhich automatically trims ".AppHost" suffix (case-insensitive)ConfigurationandHostEnvironmentproperties for direct access toIConfigurationandIHostEnvironmentImplementation Details:
AppHostEnvironment Class:
ProjectNameproperty automatically trims ".AppHost" suffix if present (case-insensitive)ConfigurationandHostEnvironmentproperties for consumers who need direct access to underlying servicesProjectDirectory,FullPath, hash values, API keys, and other AppHost-specific configurationIntegration:
AppHostEnvironmentproperty toIDistributedApplicationBuilderinterfaceDistributedApplicationBuilderconstructor and registered in DIAppHostEnvironmentinstead of directly accessingIConfigurationTest Infrastructure:
AppHostEnvironmentinTestServiceProviderso it's available in all test scenariosSimplified Dashboard Logic:
DashboardServiceto useProjectNamedirectly (suffix trimming now handled byAppHostEnvironment)ComputeApplicationNamemethod andApplicationNameRegex(no longer needed)Files Changed (36 total):
New files (1):
src/Aspire.Hosting/AppHostEnvironment.cs- Public sealed classProduction code updated (26 files):
Test infrastructure updated (10 files):
Difference from PR #13018:
The original PR used an
IAppHostEnvironmentinterface with an internalAppHostEnvironmentimplementation. This PR makesAppHostEnvironmenta public sealed class directly, eliminating the interface layer as requested. Additionally:ConfigurationandHostEnvironmentas public properties for direct accessProjectNameproperty that automatically trims ".AppHost" suffix (unified, no separateDashboardApplicationName)Fixes # (related to PR #13018)
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplateOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.