Add Http Service Resource #1030
Conversation
dotnet-issue-labeler
bot
added
the
area-app-model
|
@DamianEdwards @davidfowl Since this is planned to be in Preview2, could I have this PR reviewed please, I have free time on Thursday and Friday and I would like to finish this issue in time. |
Kahbazi
force-pushed
the
kahbazi/AddHttpService
branch
from
e8e6735
to
330f315
Compare
|
Can you generate the manifest for the app you modified (we'll likely undo that change before this gets merged). |
|
Looking at this PR I wonder why we even need HttpServiceResource. Maybe WithReference taking a URI is all we need. In the manifest, we would just hard code the url. Thoughts? |
samples/eShopLite/AppHost/Program.cs
Outdated
| @@ -1,5 +1,7 @@ | |||
| var builder = DistributedApplication.CreateBuilder(args); | |||
|
|
|||
| var petstore = builder.AddHttpService("petstore", new Uri("https://petstore.swagger.io")); | |||
There was a problem hiding this comment.
Any thoughts on what public API should be in the sample?
There was a problem hiding this comment.
None. We should bake write a test instead.
|
Let’s treat this like it’s not a custom resource at all. It’s just a way to reference and environment variable in the format service discovery expects |
| @@ -0,0 +1,29 @@ | |||
| { | |||
There was a problem hiding this comment.
@davidfowl Here's the new manifest.json. I don't have docker on my machine so the manifest is only for the front end app and the http service. I will redo the program.cs change before the merge.
@davidfowl Do you mean like this? builder.AddProject<Projects.MyFrontend>("frontend")
.WithReference("petstore", new Uri("https://petstore.swagger.io")); |
|
Yep |
| "env": { | ||
| "OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EXCEPTION_LOG_ATTRIBUTES": "true", | ||
| "OTEL_DOTNET_EXPERIMENTAL_OTLP_EMIT_EVENT_LOG_ATTRIBUTES": "true", | ||
| "services__petstore": "https://petstore.swagger.io/" |
There was a problem hiding this comment.
I'm in favor of having the http service as a resource itself and I wanted to share my thoughts. If I understand correctly, we have to use manifest to do our own deployment. Having a specific resource for Http Service gives us the benefit to identify the external http services in order to allow firewall for those addresses.
Also we use different http service address in development and production for external services, and if multiple project reference that http service, I guess it will be easier to change the manifest from the http resource rather than env of multiple projects.
There was a problem hiding this comment.
What would a deployment tool do this with this resource?
There was a problem hiding this comment.
To be fair I don't see any scenario that not having the http resource have any limitations in the deploy but having it at the resource gives me a view of all my applications reference and dependencies and I can easily set the http service URL in one place instead of possibly multiple environment variables of my projects.
If I understand the Redis resource correctly, I can argue that the Redis behaves the same when a connection string is presented. In this case I don't want the deploy to setup a new instance of Redis, I want my app to connect to an existing Redis, right?
Not sure about this yet. This would introduce overloads of |
|
@davidfowl @DamianEdwards @mitchdenny Any other feedback that should be done in the PR? |
|
Undo the sample changes and this will be good to go. |
|
Merged. @davidfowl do you want to take this in preview 2? |
|
Yep |
I tried my best to understand the structure of the project and I successfully add a http service reference and get it working but
it needs clean-up. Also, I'm not sure if I use the correct annotation.
I have couple of questions:
I will add the tests once the implementation gets approved.
Fixes #775