Added support for prompting for parameter values in run mode #10235
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Handle prompting for parameters once they have all been processed. This also handles re-prompting until they have all been processed. - Introduced MissingParameterValueException to detect when parameters are have a missing value. - Moved all parameter processing logic into ParameterProcessor
github-actions
bot
added
the
area-app-model
…eraction handling
…ters and improve async handling
…ing and interaction service usage
…handling and parameter processing
…ingParameterValueException
davidfowl
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR centralizes parameter handling into a new ParameterProcessor, introduces MissingParameterValueException for missing parameter values, and updates existing builders and orchestrator to use this new flow.
- Tests now expect
MissingParameterValueExceptioninstead ofDistributedApplicationException. - A
ParameterProcessorclass encapsulates init/handling logic forParameterResource. - The orchestrator and builder are updated to register and invoke the new processor upfront.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/WithReferenceTests.cs | Updated exception assertion to MissingParameterValueException. |
| tests/Aspire.Hosting.Tests/WithEnvironmentTests.cs | Updated exception assertion to MissingParameterValueException. |
| tests/Aspire.Hosting.Tests/Orchestrator/ParameterProcessorTests.cs | Added comprehensive tests for parameter initialization and unresolved handling. |
| tests/Aspire.Hosting.Tests/Orchestrator/ApplicationOrchestratorTests.cs | Registered and passed ParameterProcessor in orchestrator factory helper. |
| src/Aspire.Hosting/ParameterResourceBuilderExtensions.cs | Threw MissingParameterValueException instead of generic. |
| src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs | New class with async init and interactive re-prompt loop. |
| src/Aspire.Hosting/Orchestrator/ApplicationOrchestrator.cs | Injected and invoked ParameterProcessor during startup. |
| src/Aspire.Hosting/DistributedApplicationBuilder.cs | Registered ParameterProcessor in DI container. |
| src/Aspire.Hosting/ApplicationModel/ParameterResource.cs | Extended to await a TaskCompletionSource for late-bound values. |
| src/Aspire.Hosting/ApplicationModel/MissingParameterValueException.cs | New exception type with XML docs. |
Comments suppressed due to low confidence (1)
src/Aspire.Hosting/Orchestrator/ApplicationOrchestrator.cs:440
- [nitpick] It would be beneficial to add a unit test in
ApplicationOrchestratorTeststo verify thatParameterProcessor.InitializeParametersAsyncis invoked and correctly initializes parameters during orchestration startup.
await _parameterProcessor.InitializeParametersAsync(_model.Resources.OfType<ParameterResource>()).ConfigureAwait(false);
| @@ -0,0 +1,375 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
There was a problem hiding this comment.
[nitpick] This test class is over 350 lines long, covering multiple scenarios. Consider splitting it into smaller, focused test classes (e.g., initialization tests vs. unresolved-parameter handling tests) to improve readability and maintainability.
JamesNK
reviewed
Jul 3, 2025
JamesNK
reviewed
Jul 3, 2025
JamesNK
reviewed
Jul 3, 2025
mitchdenny
reviewed
Jul 3, 2025
mitchdenny
reviewed
Jul 3, 2025
mitchdenny
reviewed
Jul 3, 2025
…lse and change PrimaryButtonText to "Save"
mitchdenny
reviewed
Jul 3, 2025
| { | ||
| private readonly List<ParameterResource> _unresolvedParameters = []; | ||
|
|
||
| public async Task InitializeParametersAsync(IEnumerable<ParameterResource> parameterResources) |
There was a problem hiding this comment.
Should this take a cancellation token?
This was referenced Jul 11, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Screen.Recording.2025-07-03.010951.mp4
Missing features
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplate