dotnet / aspire

An opinionated, cloud ready stack for building observable, production ready, distributed applications in .NET
https://learn.microsoft.com/dotnet/aspire
MIT License
3.63k stars 409 forks source link

Add API to support configuring parameter default values #5410

Closed DamianEdwards closed 1 week ago

DamianEdwards commented 3 weeks ago

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 });
davidfowl commented 2 weeks ago

@davidebbo can you look at this one?

davidebbo commented 2 weeks ago

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 commented 2 weeks ago

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 commented 1 week ago

@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 commented 1 week ago

The manifest implications are interesting. My take is:

davidebbo commented 1 week ago

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