Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR streamlines the API key setup for GitHub Models by introducing a default secret parameter ({resource}-gh-apikey) and enforcing secret validation when overriding it. Key changes include:
- Automatically creating and wiring a default secret API key parameter in
AddGitHubModel - Updating
WithApiKeyto validate that custom parameters are marked as secret and override the default - Adjusting resource constructors, tests, documentation, and the playground example to align with the new pattern
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.GitHub.Models.Tests/GitHubModelsExtensionTests.cs | Tests updated to set/check the new default -gh-apikey parameter and secret behavior. |
| src/Aspire.Hosting.GitHub.Models/README.md | Documentation revised to show default parameter pattern and custom override usage. |
| src/Aspire.Hosting.GitHub.Models/GitHubModelsExtensions.cs | Implements default API key parameter creation, secret enforcement, and overrides. |
| src/Aspire.Hosting.GitHub.Models/GitHubModelResource.cs | Resource constructor and Key property updated to accept the new parameter. |
| playground/GitHubModelsEndToEnd/GitHubModelsEndToEnd.AppHost/Program.cs | Example updated to remove manual WithApiKey and align with default behavior. |
Comments suppressed due to low confidence (3)
src/Aspire.Hosting.GitHub.Models/GitHubModelsExtensions.cs:85
- [nitpick] Consider including the actual parameter name in the exception message to make it easier to identify which parameter was misconfigured, for example:
throw new ArgumentException($"Parameter '{apiKey.Resource.Name}' must be marked as secret.", nameof(apiKey));
throw new ArgumentException("The API key parameter must be marked as secret. Use AddParameter with secret: true when creating the parameter.", nameof(apiKey));
src/Aspire.Hosting.GitHub.Models/GitHubModelResource.cs:46
- [nitpick] Add a
<remarks>section to the XML doc forKeyto explain that this parameter must be secret and that its value is sourced from theParameters:{resource}-gh-apikeyconfiguration or theGITHUB_TOKENenvironment variable.
public ParameterResource Key { get; internal set; }
playground/GitHubModelsEndToEnd/GitHubModelsEndToEnd.AppHost/Program.cs:7
- [nitpick] Consider adding a brief comment here to remind users that the API key will be read from the
chat-gh-apikeyuser secret or theGITHUB_TOKENenvironment variable, since the manual.WithApiKey(...)call has been removed.
var chat = builder.AddGitHubModel("chat", "openai/gpt-4o-mini");
davidfowl
left a comment
There was a problem hiding this comment.
Tested this and it's really good! Great job!
|
/backport to release/9.4 |
|
Started backporting to release/9.4: https://github.com/dotnet/aspire/actions/runs/16297246116 |
| } | ||
|
|
||
| // Remove the existing API key parameter | ||
| builder.ApplicationBuilder.Resources.Remove(builder.Resource.Key); |
There was a problem hiding this comment.
Should we only remove it if it was the one we added? Say you called WithApiKey twice.
There was a problem hiding this comment.
Ah like switching. Good point!
var p0 = builder.AddParameter("p0", secret: true);
var p1 = builder.AddParameter("p1", secret: true);
var model = builder.AddGithubModel("ghm").WithApiKey(p0).WithApiKey(p1);This will remove p0 from the model. Yes we should fix this and only ever remove the built in parameter.
Description
Defines a default child API key parameter with the name
{resource}gh-apikey. Ensures API keys are set as secrets.Fixes #10331
Checklist
<remarks />and<code />elements on your triple slash comments?