Skip to content

Conversation

@davidfowl
Copy link
Member

Follow up on #7826 (comment). After a discussion with @eerhardt, we're removing this api

Copilot AI review requested due to automatic review settings March 4, 2025 02:36
@davidfowl davidfowl requested a review from mitchdenny as a code owner March 4, 2025 02:36
@davidfowl davidfowl requested a review from eerhardt March 4, 2025 02:36
@davidfowl davidfowl enabled auto-merge (squash) March 4, 2025 02:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR removes an ambiguous overload by requiring callers to use ReferenceExpression.Create when creating connection strings. Key changes include:

  • Removal of the test case “ConnectionStringResourceWithInterpolated” that used the ambiguous overload.
  • Updating all connection string creation calls to use ReferenceExpression.Create across tests and sample projects.
  • Removal of the overloaded method in ConnectionStringBuilderExtensions.cs that accepted an interpolated string handler.

Reviewed Changes

File Description
tests/Aspire.Hosting.Tests/WithReferenceTests.cs Removed overloaded test for interpolated connection string usage.
tests/Aspire.Hosting.Tests/AddParameterTests.cs Updated connection string creation to use ReferenceExpression.Create.
tests/Aspire.Hosting.Tests/WaitForTests.cs Updated connection string creation to use ReferenceExpression.Create.
playground/ParameterEndToEnd/ParameterEndToEnd.AppHost/Program.cs Updated connection string creation to use ReferenceExpression.Create.
tests/TestingAppHost1/TestingAppHost1.AppHost/Program.cs Updated connection string creation to use ReferenceExpression.Create.
src/Aspire.Hosting/ConnectionStringBuilderExtensions.cs Removed the overload accepting an interpolated string, cleaning up the API.

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

tests/Aspire.Hosting.Tests/WithReferenceTests/WithReferenceTests.cs:270

  • The removal of the 'ConnectionStringResourceWithInterpolated' test means the previous API behavior is no longer validated. Ensure that tests sufficiently cover the new usage with ReferenceExpression.Create.
[Fact] public async Task ConnectionStringResourceWithInterpolated() { ... }

src/Aspire.Hosting/ConnectionStringBuilderExtensions.cs:52

  • Verify that any documentation or internal references to the removed overload are updated so that users are aware that only ReferenceExpression.Create is supported.
/// Removed overload for AddConnectionString using ExpressionInterpolatedStringHandler.

@davidfowl davidfowl merged commit de8f67b into main Mar 4, 2025
72 checks passed
@davidfowl davidfowl deleted the davidfowl/remove-cs-overload branch March 4, 2025 03:07
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.

Thanks! LGTM

@github-actions github-actions bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Mar 10, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Apr 9, 2025
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