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] dapr change the app port depending on the app protocol #3626

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

karolz-ms
Copy link
Member

@karolz-ms karolz-ms commented Apr 11, 2024

Backpoirt #3184 to release/8.0
Fixes #2367

Customer Impact

Described in #2367. Basically, if a DAPR-enabled Aspire app uses anything other than HTTP for communication, it will fail to communicate with the DAPR sidecar, so pretty bad.

Testing

@paule96 did manual testing and added pretty comprehensive automated test coverage of the change.

Risk

Low IMO. The change is well localized and small.

Regression

Not really--the initial implementation of the feature only covered HTTP case

Microsoft Reviewers: Open in CodeFlow

* dapr change the app port depending on the app protocol

* fix endpoint annotations for dapr.

* fix the rest of the endpoints uri schemas

* Apply suggestions from code review

Co-authored-by: David Fowler <[email protected]>

* fix white spaces

* now we can define both, dapr protocol or endpoint from the configured endpoint

* add tests

* revert changes in the playground

* fix spelling issue

* add a large comment for all of this logic

* fix formatting

* Update src/Aspire.Hosting.Dapr/DaprDistributedApplicationLifecycleHook.cs

* change to switch expression fix test

* revert changes in playground

* format stuff

* fix is null and formating

* improve switch

---------

Co-authored-by: David Fowler <[email protected]>
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-codeflow for labeling automated codeflow. intentionally a different color! label Apr 11, 2024
@dotnet-policy-service dotnet-policy-service bot added the Servicing-consider Issue for next servicing release review label Apr 11, 2024
@danmoseley
Copy link
Member

Where did we end up with using HTTPS by default as well? Separate issue?

@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 11, 2024
@danmoseley
Copy link
Member

approved for obvious reasons.

@danmoseley danmoseley enabled auto-merge (squash) April 11, 2024 23:27
@danmoseley danmoseley added area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication and removed area-codeflow for labeling automated codeflow. intentionally a different color! labels Apr 11, 2024
@davidfowl
Copy link
Member

Where did we end up with using HTTPS by default as well? Separate issue?

Needs more design, post GA.

@danmoseley danmoseley merged commit f80e6ab into release/8.0 Apr 12, 2024
8 checks passed
@danmoseley danmoseley deleted the backport/pr-3184 branch April 12, 2024 00:08
@danmoseley danmoseley mentioned this pull request Apr 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 12, 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