Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Oct 1, 2025

  • Understand the issue: Parameters with dashes cannot be set via command line because the dash conflicts with command-line argument parsing
  • Modify GetParameterValue to support fallback lookup with normalized parameter name (replace - with _)
  • Add comprehensive tests to verify parameter lookup works with both original name and normalized name
  • Verify the fix works with manual testing (all 5 comprehensive tests passed)
  • Simplify normalization to apply universally, not just to default configuration keys (per @davidfowl feedback)
  • Update test to reflect new behavior where normalization applies to all configuration keys
  • Change condition to use string.IsNullOrEmpty(value) instead of value is null
  • Revert public API change (keep GetParameterValue private for patch release)
  • Duplicate minimal normalization logic in OpenAI extension with TODO comment
  • Duplicate minimal normalization logic in GitHub Models extension with TODO comment
  • Fix ParameterProcessor to only throw MissingParameterValueException when value is empty
  • Add tests for GenerateParameterDefault behavior in publish mode
  • Make GetParameterValue internal and use it in ParameterProcessor to properly distinguish configured from generated values
  • Replace obsolete parameterResource.Value with GetValueAsync in tests
  • Fix GenerateParameterDefault tests to provide proper ServiceProvider
  • Move normalization logic to shared helper method
  • Simplify to use extension method syntax everywhere

Summary

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):

  • Updated OpenAI extension to use builder.Configuration.GetValueWithNormalizedKey(configKey) instead of fully qualified static call
  • Updated GitHub Models extension to use builder.Configuration.GetValueWithNormalizedKey(configKey) instead of fully qualified static call
  • Updated ParameterResourceBuilderExtensions.GetParameterValue to use the shared GetValueWithNormalizedKey helper instead of duplicating the normalization logic
  • Removed unnecessary using 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):

  • Added GetValueWithNormalizedKey extension method to IConfigurationExtensions.cs in src/Shared
  • This method tries the exact configuration key first, then falls back to normalized key (dashes → underscores)
  • Consolidated normalization logic in one place

Fixed GenerateParameterDefault tests (ParameterProcessorTests.cs):

  • Updated tests to create proper execution context with ServiceProvider

Updated tests to use GetValueAsync (AddParameterTests.cs):

  • Replaced obsolete parameterResource.Value with await parameterResource.GetValueAsync(default)

Made GetParameterValue internal (ParameterResourceBuilderExtensions.cs, ParameterProcessor.cs):

  • Updated ParameterProcessor to use GetParameterValue to check for configured values
  • Properly distinguishes configured values from generated values

Behavior

  • No new public API: All changes are internal, suitable for a patch release
  • Consistent normalization: Parameters with dashes work with environment variables across all extensions
  • Shared implementation: Normalization logic is centralized in src/Shared/IConfigurationExtensions.cs and used consistently everywhere
  • Clean code: Uses extension method syntax for better readability
  • Backward compatible: Existing code continues to work unchanged
  • Precedence: Exact match (with dashes) always takes priority over normalized match (with underscores)
Original prompt

This section details on the original issue you should resolve

<issue_title>Unable to set parameters that contain - because it is invalid on the command line</issue_title>
<issue_description>If the parameter name contains - we should support replacing it with _ and resolving the name. The default name is specified

configurationKey ??= $"Parameters:{name}";

We should add a fallback to the normalized version (making the name compatible with the environment variable).</issue_description>

Comments on the Issue (you are @copilot in this section)

Fixes #11771

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits October 1, 2025 22:09
Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
… keys

Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Copilot AI changed the title [WIP] Unable to set parameters that contain - because it is invalid on the command line Support parameter names with dashes resolved from underscore configuration Oct 1, 2025
Copilot finished work on behalf of davidfowl October 1, 2025 22:25
Copilot AI requested a review from davidfowl October 1, 2025 22:25
@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2025

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11785

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11785"

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2025

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 11785

Or

  • Run remotely in PowerShell:
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 11785"

@davidfowl
Copy link
Member

@copilot

Aspire.Hosting.Tests.AddParameterTests.ParameterWithCustomConfigurationKey_DoesNotUseFallback
Aspire.Hosting.MissingParameterValueException : Parameter resource could not be used because configuration key 'CustomSection:my-key' is missing and the Parameter has no default value. at Aspire.Hosting.ParameterResourceBuilderExtensions.GetParameterValue(ConfigurationManager configuration, String name, ParameterDefault parameterDefault, String configurationKey) in //src/Aspire.Hosting/ParameterResourceBuilderExtensions.cs:line 220
at Aspire.Hosting.ParameterResourceBuilderExtensions.<>c__DisplayClass3_0.b__0(ParameterDefault parameterDefault) in /
/src/Aspire.Hosting/ParameterResourceBuilderExtensions.cs:line 112
at Aspire.Hosting.ApplicationModel.ParameterResource.<.ctor>b__3_0() in //src/Aspire.Hosting/ApplicationModel/ParameterResource.cs:line 30
at System.Lazy1.ViaFactory(LazyThreadSafetyMode mode) at System.Lazy1.ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor)
at System.Lazy`1.CreateValue()
at Aspire.Hosting.ApplicationModel.ParameterResource.get_ValueInternal() in /
/src/Aspire.Hosting/ApplicationModel/ParameterResource.cs:line 43
at Aspire.Hosting.ApplicationModel.ParameterResource.GetValueAsync(CancellationToken cancellationToken) in //src/Aspire.Hosting/ApplicationModel/ParameterResource.cs:line 94
at Aspire.Hosting.ApplicationModel.ParameterResource.get_Value() in /
/src/Aspire.Hosting/ApplicationModel/ParameterResource.cs:line 41
at Aspire.Hosting.Tests.AddParameterTests.ParameterWithCustomConfigurationKey_DoesNotUseFallback() in /home/runner/work/aspire/aspire/tests/Aspire.Hosting.Tests/AddParameterTests.cs:line 604
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 2, 2025

@copilot

Aspire.Hosting.Tests.AddParameterTests.ParameterWithCustomConfigurationKey_DoesNotUseFallback
...

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 CustomSection:my-key successfully resolves from CustomSection:my_key in configuration.

Copilot AI requested a review from davidfowl October 2, 2025 05:27
Copilot finished work on behalf of davidfowl October 2, 2025 05:27
@davidfowl
Copy link
Member

@copilot we need to update

OpenAI

builder.Configuration[$"Parameters:{name}-openai-apikey"] ??

GithubModels

builder.Configuration[$"Parameters:{name}-gh-apikey"] ??

These manually override the valueGetter.

…normalization

Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Oct 2, 2025

@copilot we need to update

OpenAI
...

Updated both extensions in commit f032e64. Made GetParameterValue public and updated:

  • OpenAI extension: Now uses GetParameterValue instead of direct configuration access
  • GitHub Models extension: Now uses GetParameterValue instead of direct configuration access

Both extensions now benefit from the dash-to-underscore normalization, allowing parameters like myai-openai-apikey to resolve from environment variables like Parameters__myai_openai_apikey.

Copilot finished work on behalf of davidfowl October 2, 2025 07:19
Copilot AI requested a review from davidfowl October 2, 2025 07:19
/// <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)
Copy link
Member

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.

Copy link
Contributor Author

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>
Copilot AI requested a review from davidfowl October 2, 2025 07:28
Copy link
Contributor Author

Copilot AI commented Oct 2, 2025

@copilot

🔴 Aspire.Hosting.Tests.Orchestrator.ParameterProcessorTests.InitializeParametersAsync_WithGenerateParameterDefaultInPublishMode_ThrowsWhenValueIsEmpty
...

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:

  1. Create a ServiceCollection with IConfiguration
  2. Build a ServiceProvider from the collection
  3. Use DistributedApplicationExecutionContextOptions to properly initialize the execution context with the ServiceProvider

The tests now properly simulate the real scenario where ParameterProcessor checks for configured values.

Copilot finished work on behalf of davidfowl October 2, 2025 15:05
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);
Copy link
Member

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...

Copy link
Contributor Author

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.

@davidfowl
Copy link
Member

@copilot move this logic that resolves the configuration value into a shared helper:

            var value = builder.Configuration[configKey];

            if (string.IsNullOrEmpty(value))
            {
                var normalizedKey = configKey.Replace("-", "_", StringComparison.Ordinal);
                value = builder.Configuration[normalizedKey];
            }

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.

@davidfowl davidfowl marked this pull request as ready for review October 2, 2025 16:33
@davidfowl davidfowl requested a review from mitchdenny as a code owner October 2, 2025 16:33
Copilot AI review requested due to automatic review settings October 2, 2025 16:33
Copy link
Contributor

Copilot AI left a 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>
Copy link
Contributor Author

Copilot AI commented Oct 2, 2025

@copilot move this logic that resolves the configuration value into a shared helper:

...

Completed in commit 3d144f1. Created GetValueWithNormalizedKey extension method in src/Shared/IConfigurationExtensions.cs and updated both OpenAI and GitHub Models extensions to use it. The shared helper:

  • First tries the exact configuration key
  • Falls back to normalized key (dashes → underscores) if value is null or empty
  • Includes XML documentation explaining the behavior

Both project files now include the shared file via Compile includes.

Copilot AI requested a review from davidfowl October 2, 2025 16:40
Copilot finished work on behalf of davidfowl October 2, 2025 16:40
Copy link
Member

@captainsafia captainsafia left a 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var value = global::Aspire.IConfigurationExtensions.GetValueWithNormalizedKey(builder.Configuration, configKey);
var value = builder.Configuration.GetValueWithNormalizedKey(configKey);

Copy link
Contributor Author

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var value = global::Aspire.IConfigurationExtensions.GetValueWithNormalizedKey(builder.Configuration, configKey);
var value = builder.Configuration.GetValueWithNormalizedKey(configKey);

Copy link
Contributor Author

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
Copy link
Member

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.

Copy link
Contributor Author

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>
Copilot finished work on behalf of captainsafia October 2, 2025 16:52
Copilot AI requested a review from captainsafia October 2, 2025 16:52
@captainsafia captainsafia enabled auto-merge (squash) October 2, 2025 17:37
@captainsafia captainsafia merged commit 4f49f9d into main Oct 2, 2025
916 of 921 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 13.0 milestone Oct 2, 2025
@captainsafia
Copy link
Member

/backport to release/9.5

@github-actions
Copy link
Contributor

github-actions bot commented Oct 2, 2025

Started backporting to release/9.5: https://github.com/dotnet/aspire/actions/runs/18201034960

radical pushed a commit that referenced this pull request Oct 2, 2025
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to set parameters that contain - because it is invalid on the command line

3 participants