-
Couldn't load subscription status.
- Fork 713
Support parameter names with dashes resolved from underscore configuration #11785
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
Support parameter names with dashes resolved from underscore configuration #11785
Conversation
Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
… keys Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11785Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11785" |
1 similar comment
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11785Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11785" |
|
Aspire.Hosting.Tests.AddParameterTests.ParameterWithCustomConfigurationKey_DoesNotUseFallback |
Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Fixed the failing test in commit 372ab25. The test has been updated to reflect the new behavior where normalization applies to all configuration keys, including custom ones. The test now verifies that |
|
@copilot we need to update OpenAI
GithubModels
These manually override the valueGetter. |
…normalization Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Updated both extensions in commit f032e64. Made
Both extensions now benefit from the dash-to-underscore normalization, allowing parameters like |
| /// <param name="configurationKey">Optional custom configuration key. If null, uses "Parameters:{name}".</param> | ||
| /// <returns>The parameter value.</returns> | ||
| /// <exception cref="MissingParameterValueException">Thrown when the parameter value cannot be found.</exception> | ||
| public static string GetParameterValue(ConfigurationManager configuration, string name, ParameterDefault? parameterDefault, string? configurationKey = null) |
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.
@copilot this change is good, but we don't want to expose any public API here because this will be in a patch release. Instead, lets duplicate the minimal logic in github models and open ai and leave a comment to refactor this in the next major version.
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.
Reverted the public API change in commit bfa3c7c. The GetParameterValue method is now private again. Duplicated the minimal normalization logic directly in both OpenAI and GitHub Models extensions with TODO comments to refactor in the next major version. Both extensions now support dash-to-underscore normalization without introducing any new public API surface.
Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Fixed in commit 0b585a8. The tests were failing because the ParameterProcessor needs access to IConfiguration through the ServiceProvider, but the tests were creating execution contexts without a ServiceProvider. Updated both tests to:
The tests now properly simulate the real scenario where ParameterProcessor checks for configured values. |
| throw new MissingParameterValueException("GenerateParameterDefault is not supported in this context. Falling back to prompting."); | ||
| // Try to get a configured value (without using the default) to see if the parameter was actually specified | ||
| var configuration = executionContext.ServiceProvider.GetRequiredService<IConfiguration>(); | ||
| ParameterResourceBuilderExtensions.GetParameterValue(configuration, parameterResource.Name, parameterDefault: null, parameterResource.ConfigurationKey); |
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.
It's not clear to me why we call this but discard the value...
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.
The call is used to check if a configured value exists (without triggering default value generation). If the value is missing, it throws MissingParameterValueException, which gets caught and handled. The throw is the important part - we want to know if there's no configured value. Addressed in subsequent commits by assigning the result to value.
|
@copilot move this logic that resolves the configuration value into a shared helper: That shared helper should be moved to https://github.com/dotnet/aspire/tree/main/src/Shared and used by src/Aspire.Hosting.GitHub.Models/GitHubModelsExtensions.cs and src/Aspire.Hosting.OpenAI/OpenAIExtensions.cs. |
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 implements support for parameter names containing dashes (-) to be resolved from configuration where dashes are replaced with underscores (_). This addresses the issue where parameters with dashes cannot be set via command line because the dash conflicts with command-line argument parsing.
- Modified parameter value resolution to support fallback lookup with normalized parameter names (replacing
-with_) - Added comprehensive test coverage for parameter lookup behavior with both original and normalized names
- Fixed test infrastructure to properly support async parameter value retrieval and provide correct ServiceProvider setup
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/Aspire.Hosting/ParameterResourceBuilderExtensions.cs |
Added fallback logic to GetParameterValue that tries underscore-normalized keys when dash keys fail |
src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs |
Updated to use GetParameterValue to distinguish configured from generated values in publish mode |
src/Aspire.Hosting.OpenAI/OpenAIExtensions.cs |
Duplicated normalization logic for OpenAI parameter resolution with TODO for future refactoring |
src/Aspire.Hosting.GitHub.Models/GitHubModelsExtensions.cs |
Duplicated normalization logic for GitHub Models parameter resolution with TODO for future refactoring |
tests/Aspire.Hosting.Tests/AddParameterTests.cs |
Added comprehensive tests for dash-to-underscore parameter resolution using async patterns |
tests/Aspire.Hosting.Tests/Orchestrator/ParameterProcessorTests.cs |
Added tests for GenerateParameterDefault behavior with proper ServiceProvider setup |
Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
... Completed in commit 3d144f1. Created
Both project files now include the shared file via Compile includes. |
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.
@copilot More fixes.
| throw new MissingParameterValueException($"GitHub API key parameter '{name}-gh-apikey' is missing and GITHUB_TOKEN environment variable is not set."), | ||
| { | ||
| var configKey = $"Parameters:{name}-gh-apikey"; | ||
| var value = global::Aspire.IConfigurationExtensions.GetValueWithNormalizedKey(builder.Configuration, configKey); |
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.
| var value = global::Aspire.IConfigurationExtensions.GetValueWithNormalizedKey(builder.Configuration, configKey); | |
| var value = builder.Configuration.GetValueWithNormalizedKey(configKey); |
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.
Applied in commit 4d55aff. Now uses builder.Configuration.GetValueWithNormalizedKey(configKey) extension method syntax.
| throw new MissingParameterValueException($"OpenAI API key parameter '{name}-openai-apikey' is missing and OPENAI_API_KEY environment variable is not set."), | ||
| { | ||
| var configKey = $"Parameters:{name}-openai-apikey"; | ||
| var value = global::Aspire.IConfigurationExtensions.GetValueWithNormalizedKey(builder.Configuration, configKey); |
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.
| var value = global::Aspire.IConfigurationExtensions.GetValueWithNormalizedKey(builder.Configuration, configKey); | |
| var value = builder.Configuration.GetValueWithNormalizedKey(configKey); |
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.
Applied in commit 4d55aff. Now uses builder.Configuration.GetValueWithNormalizedKey(configKey) extension method syntax.
| configurationKey ??= $"Parameters:{name}"; | ||
| return configuration[configurationKey] | ||
|
|
||
| // First try to get the value with the exact configuration key |
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.
Use the GetValueWithNormalizedKey shared code here.
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.
Applied in commit 4d55aff. GetParameterValue now uses configuration.GetValueWithNormalizedKey(configurationKey) instead of duplicating the normalization logic.
…files Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
|
/backport to release/9.5 |
|
Started backporting to release/9.5: https://github.com/dotnet/aspire/actions/runs/18201034960 |
…ation (#11785) * Initial plan * Add parameter name normalization support for dashes to underscores Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Refine parameter normalization to only apply to default configuration keys Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Simplify normalization logic to apply to all configuration keys Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Update src/Aspire.Hosting/ParameterResourceBuilderExtensions.cs * Make GetParameterValue public and update OpenAI/GitHub Models to use normalization Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Revert public API and duplicate normalization logic in extensions Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Fix ParameterProcessor to skip GenerateParameterDefault exception for non-empty values Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Add tests for GenerateParameterDefault behavior in publish mode Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Make GetParameterValue internal and use it in ParameterProcessor to distinguish configured from generated values Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Apply suggestions from code review * Replace obsolete parameterResource.Value with GetValueAsync in tests Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Fix GenerateParameterDefault tests to provide ServiceProvider with IConfiguration Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Update src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs * Update src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs * Move normalization logic to shared helper in IConfigurationExtensions Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> * Use extension method syntax for GetValueWithNormalizedKey across all files Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> Co-authored-by: David Fowler <davidfowl@gmail.com> Co-authored-by: captainsafia <1857993+captainsafia@users.noreply.github.com>
-with_)string.IsNullOrEmpty(value)instead ofvalue is nullSummary
This PR implements support for parameter names containing dashes (
-) to be resolved from configuration where dashes are replaced with underscores (_).Latest Changes
Simplified extension method usage (OpenAI, GitHub Models, ParameterResourceBuilderExtensions):
builder.Configuration.GetValueWithNormalizedKey(configKey)instead of fully qualified static callbuilder.Configuration.GetValueWithNormalizedKey(configKey)instead of fully qualified static callParameterResourceBuilderExtensions.GetParameterValueto use the sharedGetValueWithNormalizedKeyhelper instead of duplicating the normalization logicusing Aspire;directives (the extension method is available through the linked shared file)This makes the code cleaner and more idiomatic by using extension method syntax consistently across all files.
Previous Changes
Moved normalization logic to shared helper (
IConfigurationExtensions.cs, OpenAI, GitHub Models):GetValueWithNormalizedKeyextension method toIConfigurationExtensions.csinsrc/SharedFixed GenerateParameterDefault tests (
ParameterProcessorTests.cs):Updated tests to use GetValueAsync (
AddParameterTests.cs):parameterResource.Valuewithawait parameterResource.GetValueAsync(default)Made
GetParameterValueinternal (ParameterResourceBuilderExtensions.cs,ParameterProcessor.cs):GetParameterValueto check for configured valuesBehavior
src/Shared/IConfigurationExtensions.csand used consistently everywhereOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.