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

Resolve DistributedApplicationResourceBuilder<ResourceWithConnectionStringSurrogate> correctly in args evaluation #7716

Conversation

adamint
Copy link
Member

@adamint adamint commented Feb 20, 2025

Description

ResourceWithConnectionStringSurrogate is wrapped in its builder when we are processing args, and so we should evaluate the underlying resource.

The right fix might be to do that for all DistributedApplicationResourceBuilder, but I'm not confident that would be correct. I can't find other cases where a builder is passed instead of the resource, does anyone else know? @mitchdenny for thoughts? keeping this as a draft during discussion

Fixes #7662 (comment)

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
  • 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?

…tringSurrogate> correctly in args evaluation
@adamint adamint added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Feb 20, 2025
@mitchdenny
Copy link
Member

So I had a look at .NET Aspire 9.0 and this problem still existed there so this isn't a regression. But it is a gap that we need to fill. I think your solution is fine, but it needs some test cases to make sure that we don't accidentally regress this behavior.

@adamint adamint marked this pull request as ready for review February 20, 2025 23:33
@Copilot Copilot bot review requested due to automatic review settings February 20, 2025 23:33

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.

@adamint adamint requested a review from mitchdenny February 20, 2025 23:48
Copy link
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for jumping on this @adamint

@mitchdenny mitchdenny merged commit a757b05 into dotnet:main Feb 21, 2025
70 checks passed
@mitchdenny
Copy link
Member

/backport to release/9.1

Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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