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

Remove With methods that take a connection string #62

Closed
eerhardt opened this issue Oct 4, 2023 · 7 comments · Fixed by #84
Closed

Remove With methods that take a connection string #62

eerhardt opened this issue Oct 4, 2023 · 7 comments · Fixed by #84
Assignees
Labels
area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication

Comments

@eerhardt
Copy link
Member

eerhardt commented Oct 4, 2023

Instead, when connecting to an existing resource that the DevHost doesn't manage, the connection string should be passed to the .AddXXX method when defining the component. The the .WithXXX methods only take the component and optionally a connectionName.

For example, today we have:

    public static IDistributedApplicationComponentBuilder<T> WithRedis<T>(this IDistributedApplicationComponentBuilder<T> projectBuilder, IDistributedApplicationComponentBuilder<RedisContainerComponent> redisBuilder, string? connectionName = null)
        
    public static IDistributedApplicationComponentBuilder<T> WithRedis<T>(this IDistributedApplicationComponentBuilder<T> projectBuilder, string connectionName, string connectionString)

We just need the first method. And then the Add method:

    public static IDistributedApplicationComponentBuilder<RedisContainerComponent> AddRedisContainer(this IDistributedApplicationBuilder builder, string name, int? port = null)

Would get an overload for directly connecting to an existing Redis instance:

    public static IDistributedApplicationComponentBuilder<RedisContainerComponent> AddRedis(this IDistributedApplicationBuilder builder, string name, string connectionString)

One design decision: We could name this new method Use instead of Add. Ex. instead of builder.AddRedis("cache", "localhost:3679"), we could have builder.UseRedis("cache", "localhost:3679").

@DamianEdwards @davidfowl @mitchdenny

@eerhardt eerhardt self-assigned this Oct 4, 2023
@mitchdenny
Copy link
Member

I was going to suggest something like this. When I was looking at setting up manifest publishing I was considering that I would have to inject a placeholder Redis component (for example) to hold the connection string so that it could get emitted to manifest.

I think that this is a good suggestion.

@eerhardt
Copy link
Member Author

eerhardt commented Oct 4, 2023

One design decision: We could name this new method Use instead of Add. Ex. instead of builder.AddRedis("cache", "localhost:3679"), we could have builder.UseRedis("cache", "localhost:3679").

@davidfowl suggested offline that we stick with Add and not use Use.

I'll start working on this tomorrow after my current change is done.

@mitchdenny
Copy link
Member

One question, we currently have AddRedisContainer(...) ... are you going to have the method named differently (i.e. AddRedis(...). I am wondering whether we should be considering renaming AddRedisContainer(...) to AddRedis(...) for consistency ... or perhaps have AddRedisConnection(...)?

@eerhardt
Copy link
Member Author

eerhardt commented Oct 4, 2023

We talked about this. We are purposefully going to leave AddRedisContainer using "container" in the name, as that indicates it is a container that gets spun up locally.

However, I don't think there was consensus on whether the new method would be AddRedis or AddRedisConnection. I think there are advantages either way. What I remember was that we would just use AddRedis(name, connectionString). But I could be convinced to name it AddRedisConnection if someone feels strongly.

@DamianEdwards
Copy link
Member

DamianEdwards commented Oct 4, 2023

I think one step at a time is fine, i.e. let's do this with the existing AddRedisContainer and AddRedis names and we can get a sense of how it feels for the spectrum of scenarios we discussed and whether further differentiation via something like AddRedisConnection is warranted.

@davidfowl
Copy link
Member

davidfowl commented Oct 5, 2023

I think we may eventually land on a single name but lets get to the end and make all of the consumption consistent (connection strings and names of resources). We're landing in a really good place here with respect to the pattern.

eerhardt added a commit that referenced this issue Oct 5, 2023
Instead, add new Add overloads that take a connection string to an existing server.

Fix #62
@mitchdenny
Copy link
Member

mitchdenny commented Oct 6, 2023

I would prefer just having AddRedis(...) in the future since we can disambiguate adequately using overloads and it'll "just work". I agree that we can do this incrementally though once we get a sense of how it hangs together.

eerhardt added a commit that referenced this issue Oct 6, 2023
* Remove ConnectionString on With methods in Hosting App Model

Instead, add new Add overloads that take a connection string to an existing server.

Fix #62

* Respond to PR feedback

- Write the connectionString to the manifest if it is provided.
- Use DistributedApplicationException
@danmoseley danmoseley added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Oct 12, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 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.

5 participants