Skip to content

PublishAsConnectionString#2052

Merged
davidfowl merged 1 commit intomainfrom
davidfowl/publishas
Feb 22, 2024
Merged

PublishAsConnectionString#2052
davidfowl merged 1 commit intomainfrom
davidfowl/publishas

Conversation

@davidfowl
Copy link
Copy Markdown
Contributor

@davidfowl davidfowl commented Feb 2, 2024

Building on top of Parameter support, we can now add PublishAsConnectionString.

var redis = builder.AddRedis("cache").PublishAsConnectionString();

builder.AddProject<Projects.Api>("api");
           .WithReference(redis);

The above will use a container when running but will publish a parameter to the manifest. This is a short cut for the very common scenario:

var redis = builder.ExecutionContext.Operation switch
{
    DistributedApplicationOperation.Run => builder.AddRedis("redis"),
    _ => builder.AddConnectionString("redis")
};

builder.AddProject<Projects.Api>("api");
           .WithReference(redis);

Contributes to #1960

Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Feb 2, 2024
@davidfowl davidfowl changed the title WIP WIP: PublishAs Feb 2, 2024
@davidfowl davidfowl requested review from BrennanConroy, eerhardt and mitchdenny and removed request for BrennanConroy and mitchdenny February 2, 2024 20:35
@davidfowl
Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl davidfowl marked this pull request as ready for review February 10, 2024 10:26
Copy link
Copy Markdown
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Tests?

@mitchdenny
Copy link
Copy Markdown
Member

There is a wrinkle with this approach. Look at the manifest fragment it generates:

image

The pg9 resource is a parameter.v0 (good) but that means the value with the connection string is value not connectionString. Which means that when AZD goes to build the database connection string it is probably going to have an issue. We would need the db9 resource to have a connectionString property value that looks like this "{pg9.value};Database=db9"

@mitchdenny
Copy link
Copy Markdown
Member

Actually we could possibly just modify ParameterResource to emit a connectionString when it is used like this.

@davidfowl
Copy link
Copy Markdown
Contributor Author

Actually we could possibly just modify ParameterResource to emit a connectionString when it is used like this.

Yes this is what I am thinking too. I originally added this whole surrogate thing, but I think WithReference should just stay the same and we should change how we emit the Parameter to make it work like magic.

@davidfowl davidfowl changed the title WIP: PublishAs PublishAsConnectionString Feb 19, 2024
@davidfowl
Copy link
Copy Markdown
Contributor Author

OK this is ready for another round of review.

@mitchdenny
Copy link
Copy Markdown
Member

One minor tweak suggested. This looks good, but needs to be tested with azd before merge if it hasn't already.

/// Configures the manifest writer for this resource to be a parameter resource.
/// </summary>
/// <param name="builder">The <see cref="IResourceBuilder{T}"/>.</param>
public static void ConfigureConnectionStringManifestPublisher(IResourceBuilder<IResourceWithConnectionString> builder)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this need to be public?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, other resources might want to expose this. Right now, the other extension method is constrained to ContainerResource, IResourceWithConnectionString

- This is a shortcut for writing the resource
as an external entity in the manifest.
@davidfowl davidfowl enabled auto-merge (squash) February 22, 2024 03:28
@davidfowl davidfowl mentioned this pull request Feb 22, 2024
7 tasks
@davidfowl
Copy link
Copy Markdown
Contributor Author

/azp run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl davidfowl merged commit 4360239 into main Feb 22, 2024
@davidfowl davidfowl deleted the davidfowl/publishas branch February 22, 2024 10:06
This was referenced Feb 24, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 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.

4 participants