Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14459Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14459" |
There was a problem hiding this comment.
Pull request overview
This PR fixes broken allocated-endpoint snapshot reuse by introducing a new upsert + async retrieval API on NetworkEndpointSnapshotList, and updates existing call sites/tests to use the new API while obsoleting the old TryAdd() method.
Changes:
- Add
AddOrUpdateAllocatedEndpoint(...)andGetAllocatedEndpointAsync(...); markTryAdd(...)obsolete. - Update
EndpointReferenceExpressionresolution to await allocated endpoints via the new async API. - Refactor DCP executor logic and multiple tests to use the new upsert API instead of manually creating
ValueSnapshot<AllocatedEndpoint>instances.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Aspire.Hosting.Tests/WithEnvironmentTests.cs | Switches tests from TryAdd(ValueSnapshot) to AddOrUpdateAllocatedEndpoint(...). |
| tests/Aspire.Hosting.Tests/ExpressionResolverTests.cs | Updates container-network allocated endpoint setup to use the new API. |
| tests/Aspire.Hosting.Tests/EndpointReferenceTests.cs | Updates allocated endpoint setup to use the new API in property-resolution tests. |
| tests/Aspire.Hosting.Redis.Tests/AddRedisTests.cs | Replaces snapshot construction + TryAdd with AddOrUpdateAllocatedEndpoint(...). |
| tests/Aspire.Hosting.Qdrant.Tests/AddQdrantTests.cs | Replaces snapshot construction + TryAdd with AddOrUpdateAllocatedEndpoint(...). |
| tests/Aspire.Hosting.PostgreSQL.Tests/PostgresMcpBuilderTests.cs | Replaces snapshot construction + TryAdd with AddOrUpdateAllocatedEndpoint(...). |
| tests/Aspire.Hosting.Milvus.Tests/AddMilvusTests.cs | Replaces snapshot construction + TryAdd with AddOrUpdateAllocatedEndpoint(...). |
| tests/Aspire.Hosting.Containers.Tests/ContainerResourceTests.cs | Replaces snapshot construction + TryAdd with AddOrUpdateAllocatedEndpoint(...). |
| tests/Aspire.Hosting.Azure.Tests/AzurePostgresExtensionsTests.cs | Replaces snapshot construction + TryAdd with AddOrUpdateAllocatedEndpoint(...). |
| src/Aspire.Hosting/Dcp/DcpExecutor.cs | Uses AddOrUpdateAllocatedEndpoint(...) when populating allocated endpoints for container networks/tunnels. |
| src/Aspire.Hosting/ApplicationModel/EndpointReference.cs | Resolves allocated endpoints via GetAllocatedEndpointAsync(...) instead of TryAdd/manual snapshot logic. |
| src/Aspire.Hosting/ApplicationModel/EndpointAnnotation.cs | Introduces the new API and obsoletes TryAdd(...); constructor still wires default snapshot. |
|
@karolz-ms I've opened a new pull request, #14462, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
🎬 CLI E2E Test RecordingsThe following terminal recordings are available for commit
📹 Recordings uploaded automatically from CI run #22005273576 |
afscrome
left a comment
afscrome
left a comment
There was a problem hiding this comment.
I'll try and play around with this more when I'm home later, but one small piece of feedback that caught me out with my previous endpoint refactor PR.
|
UPDATE: done. |
eerhardt
left a comment
eerhardt
left a comment
There was a problem hiding this comment.
We should consider what we should do with DevTunnels, as we can't make the API internal with DevTunnels using it.
joperezr
left a comment
joperezr
left a comment
There was a problem hiding this comment.
This LGTM other than the one comment left. One other thing to think about (no need to block this PR for it) is to apart from introducing the obsoletes, also introducing a quick code-fixer and analyzer so people updating their packages can super easily just apply the code fixer to fix their apphosts. @DamianEdwards can correct me, but I think we have done htat in other places.
|
@joperezr RE code-fixers we only have analyzers right now, no code-fixers. |
While working on local app run and container tunnel improvements, I discovered to my dismay that the AllocatedEndpoint management API (specifically,
NetworkEndpointSnapshotList.TryAdd()is quite broken. It will not properly reuse existing snapshots. The result is that it is not possible to properly wait for an AllocatedEndpoint to appear in the system.The fix proposed here is to add a better API (namely upsert + asynchronous, cancellable get) and obsolete the old API.