Conversation
See comments in dotnet#12058
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12644Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12644" |
There was a problem hiding this comment.
Pull Request Overview
This pull request renames the property ServiceProvider to Services in the LoadInputContext class to align with naming conventions used elsewhere in the Aspire codebase (e.g., IDistributedApplicationBuilder.Services). Additionally, it refactors the WithAzureApplicationInsights extension methods to eliminate code duplication by introducing a parameterless overload that other overloads call.
- Renamed
LoadInputContext.ServiceProvidertoLoadInputContext.Servicesfor consistency - Refactored
WithAzureApplicationInsightsmethods to reduce code duplication - Updated test code to use the new property name
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Aspire.Hosting/IInteractionService.cs | Renamed LoadInputContext.ServiceProvider property to Services and updated its usage in LoadCallback instantiation |
| tests/Aspire.Hosting.Azure.Tests/ProvisioningContextProviderTests.cs | Updated test code to use the renamed Services property in both LoadInputContext and InputsDialogValidationContext instantiations |
| src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs | Refactored WithAzureApplicationInsights methods by extracting common logic into a parameterless overload |
Comments suppressed due to low confidence (1)
src/Aspire.Hosting/IInteractionService.cs:515
- The
ServiceProviderproperty inInputsDialogValidationContextshould be renamed toServicesfor consistency with the change made toLoadInputContext.Servicesand the pattern used throughout Aspire (e.g.,IDistributedApplicationBuilder.Services). This improves naming consistency across the codebase.
public required IServiceProvider ServiceProvider { get; init; }
src/Aspire.Hosting.Azure.AppService/AzureAppServiceEnvironmentExtensions.cs
Show resolved
Hide resolved
| /// <param name="builder">The AzureAppServiceEnvironmentResource to configure.</param> | ||
| /// <param name="applicationInsightsLocation">The location for Application Insights.</param> | ||
| /// <returns><see cref="IResourceBuilder{T}"/></returns> | ||
| public static IResourceBuilder<AzureAppServiceEnvironmentResource> WithAzureApplicationInsights(this IResourceBuilder<AzureAppServiceEnvironmentResource> builder, string applicationInsightsLocation) |
There was a problem hiding this comment.
Why is the location even here? This feels pretty gross 😄
There was a problem hiding this comment.
My understanding from @ShilpiRach is that app insights isn't available in all locations that AppService is. So if you deploy your app service to a region that app insights isn't in, it would fail. So this allows you to pick a different region for your app insights.
Replace should use ClearContainerFilesSources now.
…ng to Pipelines. Make DeploymentStateSection experimental.
Rename PipelineStepsExtensions to PipelineStepExtensions
|
/backport to release/13.0 |
|
Started backporting to release/13.0: https://github.com/dotnet/aspire/actions/runs/19086152386 |
See comments in #12058