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

[release/8.0-preview2] Added support for adding connection strings to non-resources #1352

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Dec 12, 2023

Backport of #1350 to release/8.0-preview2

/cc @mitchdenny @davidfowl

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Microsoft Reviewers: Open in CodeFlow

… new ConnectionString type that lets users specify the connection string name and an optional value. If there's no value then lookup happens based on the connection string name via configuration.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Dec 12, 2023
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

My 2 comments can be made in main, we don't have to block the backport PR for them.

LGTM.

src/Aspire.Hosting/Extensions/ConnectionString.cs Outdated Show resolved Hide resolved
Comment on lines +242 to +259
var connectionStringName = $"{ConnectionStringEnvironmentName}{connectionString.Name}";

return builder.WithEnvironment(context =>
{
var connectionStringValue = connectionString.Value ??
builder.ApplicationBuilder.Configuration.GetConnectionString(connectionString.Name);

if (string.IsNullOrEmpty(connectionStringValue))
{
throw new DistributedApplicationException($"A connection string for '{connectionString.Name}' could not be retrieved.");
}

if (builder.Resource is ContainerResource)
{
connectionStringValue = HostNameResolver.ReplaceLocalhostWithContainerHost(connectionStringValue, builder.ApplicationBuilder.Configuration);
}

context.EnvironmentVariables[connectionStringName] = connectionStringValue;
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 connectionStringName = $"{ConnectionStringEnvironmentName}{connectionString.Name}";
return builder.WithEnvironment(context =>
{
var connectionStringValue = connectionString.Value ??
builder.ApplicationBuilder.Configuration.GetConnectionString(connectionString.Name);
if (string.IsNullOrEmpty(connectionStringValue))
{
throw new DistributedApplicationException($"A connection string for '{connectionString.Name}' could not be retrieved.");
}
if (builder.Resource is ContainerResource)
{
connectionStringValue = HostNameResolver.ReplaceLocalhostWithContainerHost(connectionStringValue, builder.ApplicationBuilder.Configuration);
}
context.EnvironmentVariables[connectionStringName] = connectionStringValue;
return builder.WithEnvironment(context =>
{
var connectionStringValue = connectionString.Value ??
builder.ApplicationBuilder.Configuration.GetConnectionString(connectionString.Name);
if (string.IsNullOrEmpty(connectionStringValue))
{
throw new DistributedApplicationException($"A connection string for '{connectionString.Name}' could not be retrieved.");
}
if (builder.Resource is ContainerResource)
{
connectionStringValue = HostNameResolver.ReplaceLocalhostWithContainerHost(connectionStringValue, builder.ApplicationBuilder.Configuration);
}
context.EnvironmentVariables[$"{ConnectionStringEnvironmentName}{connectionString.Name}"] = connectionStringValue;

You are already capturing connectionString into the lambda, so you might as well make this name in the lambda instead of making the capture object bigger.

Copy link
Member

Choose a reason for hiding this comment

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

I prefer to have the exact same code in main and preview2.

Copy link
Member

Choose a reason for hiding this comment

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

Like I said above "My 2 comments can be made in main, we don't have to block the backport PR for them.".

@davidfowl davidfowl enabled auto-merge (squash) December 12, 2023 15:23
@davidfowl davidfowl merged commit 834b0cf into release/8.0-preview2 Dec 12, 2023
8 checks passed
@davidfowl davidfowl deleted the backport/pr-1350-to-release/8.0-preview2 branch December 12, 2023 16:04
eerhardt added a commit to eerhardt/aspire that referenced this pull request Dec 12, 2023
eerhardt added a commit that referenced this pull request Dec 12, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 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 this pull request may close these issues.

3 participants