-
Notifications
You must be signed in to change notification settings - Fork 749
Add a builder pattern for resolving resource configuration #13221
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
Add a builder pattern for resolving resource configuration #13221
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 13221Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 13221" |
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 introduces a builder pattern for resolving resource configuration (command line arguments and environment variables) with optional support for additional configuration sources like certificate trust and server authentication certificates. The goal is to create a consistent, extensible API that can accommodate new configuration contributors as execution contexts evolve.
Key changes:
- Introduces
ResourceConfigurationBuilderwith a fluent API for configuring resources - Adds configuration gatherers for arguments, environment variables, certificate trust, and server authentication certificates
- Refactors
DcpExecutorto use the new builder pattern instead of the legacyProcessConfigurationValuesAsyncmethod - Removes
WasResolvedtracking fromReferenceExpressionand moves it to metadata-specific tracking
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
DcpExecutor.cs |
Refactored to use new ResourceConfigurationBuilder API instead of ProcessConfigurationValuesAsync |
DcpExecutorTests.cs |
Added missing resourceLoggerService to test service provider |
ResourceConfigurationBuilder.cs |
New builder class implementing the builder pattern for resource configuration |
ResourceConfigurationBuilderExtensions.cs |
Extension methods for fluent API (WithArguments, WithEnvironmentVariables, etc.) |
IResourceConfigurationBuilder.cs |
Public interface for the builder pattern |
IResourceConfiguration.cs |
Public interface representing resolved configuration |
ResourceConfiguration.cs |
Internal implementation of IResourceConfiguration |
IResourceConfigurationGatherer.cs |
Public interface for configuration gatherers |
IResourceConfigurationGathererContext.cs |
Public interface for gatherer context |
ResourceConfigurationGathererContext.cs |
Internal implementation of gatherer context |
ResourceArgumentsConfigurationGatherer.cs |
Gatherer for command line arguments |
ResourceEnvironmentVariablesConfigurationGatherer.cs |
Gatherer for environment variables |
ResourceCertificateTrustConfigurationGatherer.cs |
Gatherer for certificate trust configuration |
ResourceServerAuthenticationCertificateConfigurationGatherer.cs |
Gatherer for server authentication certificates |
IResourceConfigurationMetadata.cs |
Marker interface for configuration metadata |
ResourceConfigurationExtensions.cs |
Extension methods for querying metadata |
ReferenceExpression.cs |
Removed WasResolved tracking (moved to metadata-specific implementation) |
ExpressionResolver.cs |
Removed WasResolved tracking assignment |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
...ire.Hosting/ApplicationModel/ResourceServerAuthenticationCertificateConfigurationGatherer.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/ResourceCertificateTrustConfigurationGatherer.cs
Outdated
Show resolved
Hide resolved
...ire.Hosting/ApplicationModel/ResourceServerAuthenticationCertificateConfigurationGatherer.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/ResourceConfigurationGathererContext.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/ResourceArgumentsConfigurationGatherer.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/ResourceConfigurationBuilder.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
…onCertificateConfigurationGatherer.cs Co-authored-by: Copilot <[email protected]>
…nfigurationGatherer.cs Co-authored-by: Copilot <[email protected]>
…onCertificateConfigurationGatherer.cs Co-authored-by: Copilot <[email protected]>
…rerContext.cs Co-authored-by: Copilot <[email protected]>
src/Aspire.Hosting/ApplicationModel/IResourceExecutionConfiguration.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/IResourceExecutionConfiguration.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/IResourceConfigurationGatherer.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/IResourceExecutionConfigurationGathererContext.cs
Show resolved
Hide resolved
|
This PR adds a lot of new public API surface. How confident are we in the shape of these types? |
Much more confident that the horrible aggregation function this replaces. We may want to polish up the method names and comments, but I don't think there's a more straightforward pattern to accomplish the goal of making it easier to re-use and extend the configuration resolution logic as more features are added that need to contribute. |
|
The existing resolution methods in ResourceExtensions.cs are also public out of necessity; they need to be consume by publish environments and integrations that don't ship as part of Aspire.Hosting. The long term goal would be for these APIs to replace the existing |
src/Aspire.Hosting/ApplicationModel/IResourceExecutionConfigurationBuilder.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/ProcessedResourceExecutionConfiguration.cs
Show resolved
Hide resolved
|
Help me understand why we need this? |
The recent certificates features have been needing to plug into argument and environment variable configuration in a way that meshes with the existing argument and environment configuration; the initial implementations were various kinds of ugly hacks, but that already started causing headaches in making sure everything was processed in a consistent way. I wanted to replace the ugly hacks I added onto the old environment and argument processing methods with something that worked the same way, but would give a consistent way to onboard the certificates features in publish and deployment scenarios in the future. This effectively replaces the giant function of doom from #12995 with a more holistic API for evaluating config that works everywhere (and maintains the same behavior the old one did). |
Description
Adds a builder pattern for resolving resource configuration (command line arguments and environment variables) with the ability to optionally resolve additional configuration sources (certificate trust, server authentication certificates) in a consistent API. The idea is to create something that can be consistent and allow extending with new contributors to configuration as execution contexts support them.
Fixes #13219