Account for standard environment variable prefixes when simulating launch profile #7698
Conversation
There was a problem hiding this comment.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
src/Aspire.Hosting.Testing/DistributedApplicationFactory.cs:164
- Removing this default environment assignment may lead to unexpected behavior if no environment variable is provided in the launch profile. Please ensure that the intended default (Development) is set when neither a plain nor a prefixed environment variable is present.
hostBuilderOptions.EnvironmentName = Environments.Development;
tests/Aspire.Hosting.Testing.Tests/TestingBuilderTests.cs:110
- Consider adding tests that explicitly verify how environment variables with the 'DOTNET_' and 'ASPNETCORE_' prefixes propagate into configuration, ensuring that the precedence order (no prefix > DOTNET_ > ASPNETCORE_) is correctly enforced.
[Fact]
| @@ -162,7 +162,6 @@ private static void PreConfigureBuilderOptions( | |||
| }; | |||
| applicationOptions.Args = hostBuilderOptions.Args; | |||
|
|
|||
| hostBuilderOptions.EnvironmentName = Environments.Development; | |||
There was a problem hiding this comment.
EnvironmentName is nullable. When not set, it defaults to Production. Our launch profiles set it to Development. If there is no launch profile, therefore, the new behavior would be to set it to Production by default. This would most likely come up in the ad-hoc testing case where the developer has no app host specified (the overloads of DistributedApplicationTestingBuilder.CreateAsync(...) which do not accept a type arg)
| @@ -264,6 +263,18 @@ private static void PostConfigureBuilderOptions( | |||
| foreach (var (key, value) in envVars) | |||
| { | |||
| SetDefault(key, value); | |||
|
|
|||
| // See https://github.com/dotnet/runtime/blob/8edaf7460777e791b6279b395a68a77533db2d20/src/libraries/Microsoft.Extensions.Hosting/src/HostApplicationBuilder.cs#L96 | |||
| if (key.StartsWith("DOTNET_")) | |||
There was a problem hiding this comment.
| if (key.StartsWith("DOTNET_")) | |
| if (key.StartsWith("DOTNET_", StringComparison.OrdinalIgnoreCase)) |
| } | ||
|
|
||
| // See https://github.com/dotnet/aspnetcore/blob/4ce2db7b8d85c07cad2c59242edc19af6a91b0d7/src/DefaultBuilder/src/WebApplicationBuilder.cs#L38 | ||
| if (key.StartsWith("ASPNETCORE_")) |
There was a problem hiding this comment.
| if (key.StartsWith("ASPNETCORE_")) | |
| if (key.StartsWith("ASPNETCORE_", StringComparison.OrdinalIgnoreCase)) |
Description
Environment variables prefixed with
DOTNET_orASPNETCORE_should be propagated into configuration in non-prefixed form. For example, settingDOTNET_ENVIRONMENT=Testingvia an environment variable should setENVIRONMENT=Testingin configuration.In the Aspire.Hosting.Testing package, we simulate loading the app host's launch profile, including environment variables. We do not set environment variables in the process since that would leak between concurrently running tests. Instead, we read the environment variables from the launch profile into configuration. This PR changes how we propagate the environment variables into configuration so that we check for those two well-known prefixes and flow them into configuration without those prefixes with the precedence order of no prefix >
DOTNET_>ASPNETCORE_.In particular, this allows setting the environment name from launch profile to work as expected.
Checklist
<remarks />and<code />elements on your triple slash comments?breaking-changetemplate):doc-ideatemplate):