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

#2367 dapr change the app port depending on the app protocol #3184

Merged
merged 20 commits into from
Apr 11, 2024

Conversation

paule96
Copy link
Contributor

@paule96 paule96 commented Mar 26, 2024

if you dapr connected apps uses anything else then http they will currently not correctly connecting to the sidecare. (more details to that issue in #2367 )

To fix this now, a use can set the app protocol in the sidecare options and this will then take the correct endpoint information.

Open questions:

  • Can someone test the playground project for dapr?
    • for me on Linux it seems to be broken in
Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Mar 26, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 26, 2024
@paule96
Copy link
Contributor Author

paule96 commented Mar 27, 2024

okay, I know now why the playground project is not working. There seems to be a bug unrelated to this PR that it is required to have dapr 1.13.0 as a reference for aspire >= preview 4.
The problem is there is only dapr 1.12.0 available on the feeds configured in the NuGet.config.

@paule96
Copy link
Contributor Author

paule96 commented Mar 27, 2024

Okay after I integrated locally the nuget.org feed I was able to test my change. For me it looks like it is working.

@paule96 paule96 requested a review from philliphoff April 2, 2024 06:11
@paule96
Copy link
Contributor Author

paule96 commented Apr 8, 2024

@davidfowl and @karolz-ms,

because you both were involved in #3305 can I ask you to review this PR too?

@paule96
Copy link
Contributor Author

paule96 commented Apr 10, 2024

@davidfowl please review again :) I changed the logic and also added tests

@davidfowl
Copy link
Member

This looks great

Copy link
Member

@karolz-ms karolz-ms left a comment

Choose a reason for hiding this comment

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

Looks great overall, thanks so much @paule96 for taking care of this. I have just one question regarding the use of targetPortExpression in the test

@karolz-ms karolz-ms merged commit c25ccaa into dotnet:main Apr 11, 2024
8 checks passed
@davidfowl
Copy link
Member

/backport to release/8.0

Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/aspire/actions/runs/8654197627

Copy link
Contributor

@davidfowl backporting to release/8.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: dapr change the app port depending on the app protocol
Applying: fix endpoint annotations for dapr.
error: sha1 information is lacking or useless (src/Aspire.Hosting.Dapr/DaprDistributedApplicationLifecycleHook.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 fix endpoint annotations for dapr.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

@davidfowl an error occurred while backporting to release/8.0, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

karolz-ms pushed a commit that referenced this pull request Apr 11, 2024
* 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]>
danmoseley pushed a commit that referenced this pull request Apr 12, 2024
…3626)

* 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



* 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: paule96 <[email protected]>
Co-authored-by: David Fowler <[email protected]>
@danmoseley danmoseley mentioned this pull request Apr 12, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 13, 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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants