-
Notifications
You must be signed in to change notification settings - Fork 657
Add ExternalServiceResource for modeling external services with service discovery support #9965
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
Conversation
Use the proposal from this comment on the issue: #2311 (comment) |
Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com>
… support Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com>
I've updated the implementation to follow your proposal more closely. The key changes:
The implementation now supports all the overloads from your proposal with proper URL validation. Currently addressing some integration issues with the endpoint system to ensure the URLs resolve correctly. I'm working on fixing the remaining test failures where the endpoint URLs need to preserve the original external service URL format rather than the standard |
…t integration Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com>
Does this solution also work with health checks? |
@wertzui that's a really good point. |
Updated GetPortFromUrl method to use scheme-appropriate default ports for parameterized expressions: - HTTPS: 443 (was hardcoded 80) - HTTP: 80 - FTP: 21 - WebSocket (ws): 80 - Secure WebSocket (wss): 443 - Unknown schemes: 80 (fallback) Addresses @DamianEdwards' comment about considering URL scheme for default port selection. Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com>
@copilot, please try again |
…, and update tests Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com>
I've addressed all the code review feedback:
All external service tests are now passing. The implementation follows the proposal and integrates cleanly with the existing Aspire infrastructure. Fixed in commit b28113b. |
- Removed 44 Bootstrap CSS/JS files from playground/ExternalServices/WebFrontEnd/wwwroot/lib/ - Updated App.razor to use CDN links instead of local Bootstrap files - Fixed YARP integration to work with new ExternalServiceResource.UrlExpression property - Fixed ExternalServices playground AppHost to use correct AddExternalService syntax This reduces the PR diff from +61,210 lines to a much more reasonable size by eliminating vendor CSS/JS files. Co-authored-by: JamesNK <303201+JamesNK@users.noreply.github.com>
Fixed the massive diff size! Removed 44 Bootstrap vendor files (60,000+ lines) from the playground project and switched to CDN links instead. The diff is now much more reasonable. Fixed in commit 71175bd. |
Co-authored-by: DamianEdwards <249088+DamianEdwards@users.noreply.github.com>
I've reverted the changes from commit 56848cb and updated the tests to work with the restored implementation. The ExternalServiceResource now uses the original Uri/UrlParameter approach instead of UrlExpression. Build passes successfully. Fixed in commit 6392b16. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dashboard changes are good.
I took a look at the app host stuff but I'm less fimilar with adding new resources. I'm curious whether extenal service should implement IResourceWithEndpoints
? I don't know what features that interface enables.
@JamesNK we decided against endpoints as they don't fit very ergonomically for external addresses where nothing needs to be allocated by Aspire/DCP. Instead we have specific overloads of |
@DamianEdwards some follow ups for you. Logging logs for *every instance of the same parameter. It's nosy, if you want to log then we need to find a better place to do it. I spent some time looking at how to make yarp work and we can do it once @benjaminpetit fixes the YARP publish story |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good but I'm having trouble getting it to deploy cleanly to Docker Compose (just trying to figure out if this PR breaks something or something is broken in main.
@davidfowl what issue did you have with Yarp publishing? I managed to get it running locally on Windows with Docker Compose up? |
Pleased to report that the issue I was having on the branch resolved itself once this was merged onto main (yay?) |
Container files don’t get resolved outside of docker compose scenarios and there are issues with URLs as parameters in yarp. We’re switching to env variables so it works everywhere |
This PR introduces a new
ExternalServiceResource
type that allows developers to model external services (APIs, databases, etc.) in their Aspire applications with service discovery support.Problem
Previously, developers had to create custom implementations to represent external services in their Aspire applications:
Solution
Added
ExternalServiceResource
andAddExternalService
extension methods that provide a clean, built-in way to model external services:Features
WithReference()
- works seamlessly with existing service discovery infrastructureFixes #2311
Screenshots of external resources in the dashboard