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

[release/8.0] Added ability to resolve target port #3445

Merged
merged 3 commits into from
Apr 6, 2024

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Apr 5, 2024

Backport of #3305 #3371 and #3372 to release/8.0

/cc @davidfowl

Customer Impact

Dapr was broken in preview5 because of the endpoint / port refactoring. This changes how we process and expose target ports to correct that break. It refactors APIs that can't be changed once we ship 8.0, so it falls under "API fit and finish".

Testing

New unit tests added.
LocalOnly Node app test now passes locally
Manual testing

Risk

Medium - the original change has already broken Node apps

Microsoft Reviewers: Open in CodeFlow

davidfowl and others added 3 commits April 5, 2024 17:20
* Added ability to resolve target port
- Change how we process and expose target ports. This change makes it possible to avoid using any internals of DCP to access and resolve target ports from any resource.
- More launch profile processing to the project resource defaults.
- Remove logic in the manifest writing that wrote a special env variable. This should just work now  like everything else.
- Exposed TargetPort and Exists on EndpointReference.
- Use this new target property with dapr.
- Added a ReferenceExpressionBuilder which makes it possible to dynamically build a ReferenceExpression.
- Endpoints and env are evaulated earlier now so change remove
tests that add default launch profile on real projects
without allocating those endpoints.

* Remove more uses of TestProgram

* Fix tests
This allows us to not leak "DCP" into the public API and keeps DCP-isms contained in the DCP code.
* Do not set TargetPort to Port when IsProxied is true

* Move TargetPort check to ApplicationExecutor

* Apply port selection logic to manifest publishing

(cherry picked from commit 09f5ae9)

* Update src/Aspire.Hosting/Dcp/ApplicationExecutor.cs

Co-authored-by: Eric Erhardt <[email protected]>

---------

Co-authored-by: Eric Erhardt <[email protected]>
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Apr 5, 2024
@dotnet-policy-service dotnet-policy-service bot added the Servicing-consider Issue for next servicing release review label Apr 5, 2024
@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 6, 2024
@davidfowl
Copy link
Member

Approved in chat

@davidfowl davidfowl merged commit 5ed3614 into dotnet:release/8.0 Apr 6, 2024
8 checks passed
@danmoseley danmoseley mentioned this pull request Apr 12, 2024
@eerhardt eerhardt deleted the PortResolveTargetPort branch April 15, 2024 15:18
@github-actions github-actions bot locked and limited conversation to collaborators May 17, 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 Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants