Skip to content
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 API to support configuring parameter default values #5410

Closed
DamianEdwards opened this issue Aug 23, 2024 · 6 comments · Fixed by #5529
Closed

Add API to support configuring parameter default values #5410

DamianEdwards opened this issue Aug 23, 2024 · 6 comments · Fixed by #5529
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Milestone

Comments

@DamianEdwards
Copy link
Member

DamianEdwards commented Aug 23, 2024

Background and Motivation

Adding parameters to the application model via builder.AddParameter("name") is straightforward enough, but sometimes it's desirable to create parameters that have a default value generated via the GenerateParameterDefault type, or use a hard-coded default value string, or custom ParameterDefault implementation. This is difficult to do today as ParameterResource default behaviors are largely controlled by the body of the builder.AddParameter() method itself and that method accepts no overloads for controlling parameter default values.

There should be API that makes it simple to set a parameter's default value in the same way that the implicit password parameters for many resources are.

Related:

Proposed API

Suggestion 1: Overloads of AddParameter that accept string, GenerateParameterDefault, and ParameterDefault.

namespace Aspire.Hosting;

public static class ParameterResourceBuilderExtensions
{
+    public static IResourceBuilder<ParameterResource> AddParameter(this IDistributedApplicationBuilder builder, string name, string defaultValue, bool secret = false)
+    public static IResourceBuilder<ParameterResource> AddParameter(this IDistributedApplicationBuilder builder, string name, GenerateParameterDefault defaultValue, bool secret = true)
+    public static IResourceBuilder<ParameterResource> AddParameter(this IDistributedApplicationBuilder builder, string name, ParameterDefault defaultValue, bool secret = false)
}

The second overload above (the one that accepts GenerateParameterDefault would create the ParameterResource internally using the CreateGeneratedParameter method which ensures the generated value is stored in user secrets automatically.

Suggestion 2: Extension method to IResourceBuilder<ParameterResource> that allows setting the parameter default value

An alternative approach is to add new extension methods that enable setting the default value after the parameter is created and added to the application model with AddParameter:

public static class ParameterResourceBuilderExtensions
{
+    public static IResourceBuilder<ParameterResource> WithDefault(this IResourceBuilder<ParameterResource> builder, string defaultValue)
+    public static IResourceBuilder<ParameterResource> WithDefault(this IResourceBuilder<ParameterResource> builder, GenerateParameterDefault defaultValue)
+    public static IResourceBuilder<ParameterResource> WithDefault(this IResourceBuilder<ParameterResource> builder, ParameterDefault defaultValue)
}

The second overload above (the one that accepts GenerateParameterDefault would wrap it in an instance of the UserSecretsParameterDefault internal class which ensures the generated value is stored in user secrets automatically.

Note that this approach would likely require some reworking of how AddParameter is currently implemented as it currently defaults the parameter resource initial state to an error if a value isn't found in configuration.

Usage Examples

Suggestion 2: AddParameter overloads

var keycloakRealmName = builder.AddParameter("keycloak-realm", "SampleRealm");
var keycloakRealmDisplayName = builder.AddParameter("keycloak-realm-display", "Sample Keycloak Realm");
var webBlazorSsrClientId = builder.AddParameter("web-blazorssr-client-id", "web.blazorssr");
var webBlazorSsrClientName = builder.AddParameter("web-blazorssr-client-name", "Blazor SSR Web App");
var apiWeatherClientId = builder.AddParameter("api-weather-client-id", "api.weather");
var apiWeatherClientName = builder.AddParameter("api-weather-client-name", "Weather API App");
var webBlazorSSRClientSecret = builder.AddParameter("web-blazorssr-client-secret", new GenerateParameterDefault { MinLength = 32, Special = false }, secret: true);
var apiWeatherClientSecret = builder.AddParameter("api-weather-client-secret", new GenerateParameterDefault { MinLength = 32, Special = false }, secret: true);

Suggestion 2: WithDefault

var keycloakRealmName = builder.AddParameter("keycloak-realm").WithDefault("SampleRealm");
var keycloakRealmDisplayName = builder.AddParameter("keycloak-realm-display").WithDefault("Sample Keycloak Realm");
var webBlazorSsrClientId = builder.AddParameter("web-blazorssr-client-id").WithDefault("web.blazorssr");
var webBlazorSsrClientName = builder.AddParameter("web-blazorssr-client-name").WithDefault("Blazor SSR Web App");
var apiWeatherClientId = builder.AddParameter("api-weather-client-id").WithDefault("api.weather");
var apiWeatherClientName = builder.AddParameter("api-weather-client-name").WithDefault("Weather API App");
var webBlazorSSRClientSecret = builder.AddParameter("web-blazorssr-client-secret", secret: true)
    .WithDefault(new GenerateParameterDefault { MinLength = 32, Special = false });
var apiWeatherClientSecret = builder.AddParameter("api-weather-client-secret", secret: true)
    .WithDefault(new GenerateParameterDefault { MinLength = 32, Special = false });
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Aug 23, 2024
@davidfowl
Copy link
Member

@davidebbo can you look at this one?

@davidebbo
Copy link
Contributor

This has some overlap with #4429. e.g. the first overload there is the same as the first from Suggestion 1 here. But the rest is distinct. How do we rationalize this? We sort of want the union of both proposals?

See also my comment there, as the same questions apply here. In particular, the 'Do not add multiple overloads with optional parameters' is going to hit us. Suggestion 2 probably avoids it.

Other question: why have distinct overloads for GenerateParameterDefault and ParameterDefault. Given that the former extends the latter, isn't the latter one sufficient?

@DamianEdwards
Copy link
Member Author

Other question: why have distinct overloads for GenerateParameterDefault and ParameterDefault. Given that the former extends the latter, isn't the latter one sufficient?

The feature that saves generated values to the user secrets store relies on GenerateParameterDefault so I went with a concrete overload to enable that. We could just downcast instead though.

@davidebbo
Copy link
Contributor

@DamianEdwards and to be clear, everything you're discussing here is a fallback value, right? i.e. if the setting is found in app settings, we use that as normal. Otherwise, we use this, instead of not having a value at all.

That seems like a very simple change, and I'll take a crack at it.

I like Solution 1 better. And I think the 'Do not add multiple overloads with optional parameters' warning it gives is bogus in this case, since all the overloads are well differentiated by param types.

@davidebbo
Copy link
Contributor

The manifest implications are interesting. My take is:

  • For the ParameterDefault overload, it will end up in the manifest. At least for GenerateParameterDefault, we know that generates a valid schema. Alternate ParameterDefault implementations are probably hard to support right now without schema changes.
  • For the string defaultValue overload, it probably shouldn't generate anything at all. Looking at https://json.schemastore.org/aspire-8.0.json, generate seems like the only valid value for default. This matches what @davidfowl says in AddParameter overloads that supports providing a value  #4429 about "Adding a parameter with a default value only used at runtime"

@davidebbo
Copy link
Contributor

davidebbo commented Sep 3, 2024

Ok, PR #5529 is out. Let's start with that before digging into #4429, which has more open questions.

@DamianEdwards DamianEdwards added this to the 9.0 milestone Sep 5, 2024
davidfowl pushed a commit that referenced this issue Sep 6, 2024
#5529)

* Add AddParameter overloads that take a constant and a ParameterDefault

Fixes #5410

* Persist the default value if it needs it

* Add test that checks that UserSecretsParameterDefault is only used when it should be

* Tweak previous test

* Change behavior so that value provided in code is always used

* Rename tests to match new behavior

* Add a persist param to AddParameter

* Fix comment

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

---------

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@github-actions github-actions bot locked and limited conversation to collaborators Oct 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants