Skip to content

Conversation

adamint
Copy link
Member

@adamint adamint commented Feb 20, 2025

Description

Duplicates logic from ConnectionStringReference#GetValueAsync to avoid evaluating the resource connection string expression twice.

Fixes https://github.com/dotnet/aspire/pull/7662/files/f36a730ab8fdc79867669daed643a5bc1a2a6f52#r1962999417

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No, covered by existing tests
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?

@Copilot Copilot AI review requested due to automatic review settings February 20, 2025 17:51
Copy link
Contributor

@Copilot 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.

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

Comments suppressed due to low confidence (1)

src/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs:160

  • [nitpick] The variable name 'value' is ambiguous in this context. Consider renaming it to a more descriptive name such as 'resolvedConnectionString' to improve code clarity.
var value = await ResolveInternalAsync(cs.Resource.ConnectionStringExpression).ConfigureAwait(false);

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.

LGTM

@adamint
Copy link
Member Author

adamint commented Feb 20, 2025

/backport to release/9.1

Copy link
Contributor

Started backporting to release/9.1: https://github.com/dotnet/aspire/actions/runs/13441849725

@adamint adamint merged commit 11ba9fe into dotnet:main Feb 20, 2025
70 checks passed
@adamint
Copy link
Member Author

adamint commented Feb 20, 2025

/backport to release/9.1

Copy link
Contributor

Started backporting to release/9.1: https://github.com/dotnet/aspire/actions/runs/13442202094

@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.

2 participants