-
Notifications
You must be signed in to change notification settings - Fork 66
Convert samples to use Aspire #593
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
This updates the samples to use Aspire for connecting the client ASP.NET Core and the server ASP.NET Framework applications. This does the following: - Separate out Directory.Packages.props for samples/src/test. The src one pins all the dependencies so that we know which versions are being used, while the test/samples do not so they can run in a more hybrid setup - Update the folder structure to keep samples together - Update playwright tests to connect via Aspire - Ensure each sample has a smoke test (either via HttpClient or Playwright) - Use libman for managing sample javascript/css files
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.
Pull Request Overview
This PR updates the samples to use Aspire for connecting ASP.NET Core clients with ASP.NET Framework servers. Key changes include reorganizing sample folders and project references, updating configuration keys and launch settings, and revising solution and build properties for the new setup.
- Updated launch settings and project files in AuthRemoteFormsAuth samples.
- Changed configuration keys for reverse proxy setup in AuthRemoteFormsAuthCore.
- Revised solution and global build files to reflect new dependency and packaging strategies.
Reviewed Changes
Copilot reviewed 490 out of 490 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
samples/AuthRemoteFormsAuth/AuthRemoteFormsAuthCore/Properties/launchSettings.json | Adds launch settings for the ASP.NET Core sample. |
samples/AuthRemoteFormsAuth/AuthRemoteFormsAuthCore/Program.cs | Refactors service configuration and reverse proxy mapping using updated configuration keys. |
samples/AuthRemoteFormsAuth/AuthRemoteFormsAuthCore/AuthRemoteFormsAuthCore.csproj | Updates target framework and project references. |
samples/AuthRemoteFormsAuth/AuthRemoteFormsAuthAppHost/appsettings.json | Introduces logging configuration with Aspire-specific log levels. |
samples/AuthRemoteFormsAuth/AuthRemoteFormsAuthAppHost/Properties/launchSettings.json | Provides new launch settings for the AppHost sample. |
samples/AuthRemoteFormsAuth/AuthRemoteFormsAuthAppHost/Program.cs | Configures the distributed application builder for the AppHost sample. |
samples/AuthRemoteFormsAuth/AuthRemoteFormsAuthAppHost/AuthRemoteFormsAuthAppHost.csproj | Updates the AppHost project file to leverage the Aspire host SDK. |
global.json | Adjusts dotnet and SDK versions and adds new SDK references. |
Microsoft.AspNetCore.SystemWebAdapters.slnf | Modifies the solution file to include the new samples and package props. |
Microsoft.AspNetCore.SystemWebAdapters.sln | Reorganizes project nesting and removes outdated test project references. |
Directory.Build.props | Adds a packaging property for running pack on the solution. |
Comments suppressed due to low confidence (1)
samples/AuthRemoteFormsAuth/AuthRemoteFormsAuthCore/Program.cs:14
- The configuration keys have been updated from 'ProxyTo'/'RemoteAppApiKey' to 'RemoteApp:Url' and 'RemoteApp:ApiKey'. Please confirm that all consumers are aware of this API change and that documentation is updated accordingly.
options.RemoteAppUrl = new(builder.Configuration["RemoteApp:Url"]!);
Co-authored-by: Copilot <[email protected]>
|
||
var remoteApiKey = builder.AddParameter("apiKey", Guid.NewGuid().ToString(), secret: true); | ||
|
||
var frameworkApp = builder.AddIISExpress("iis") |
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.
TIL about these extensions! Preety neat!
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.
Yes - thanks to some great work by @CZEMacLeod!
|
||
var coreApp = builder.AddProject<Projects.AuthRemoteFormsAuthCore>("core") | ||
.WithEnvironment("RemoteApp__ApiKey", remoteApiKey) | ||
.WithEnvironment("RemoteApp__Url", frameworkApp.GetEndpoint("https")) |
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.
Assuming the resource returned back from the above is not an IResourceWithEnvironment? Otherwise you could just do a .WithReference(frameworkApp)
here
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.
cool. I'll get this merged in and then look at what can make the aspire experience better here
app.MapForwarder("/{**catch-all}", app.Configuration["ProxyTo"]!).WithOrder(int.MaxValue); | ||
|
||
// Configure the reverse proxy to forward all unhandled requests to the remote app | ||
app.MapForwarder("/{**catch-all}", app.Configuration["RemoteApp:Url"]!) |
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.
NIT: If you want to also have HealthChecks enabled, you could also call here the app.MapDefaultEndpoints()
from your ServiceDefaults project which should do that.
var frameworkApp = iisExpress.AddSiteProject<Projects.AuthRemoteIdentityFramework>("framework") | ||
.WithDefaultIISExpressEndpoints() | ||
.WithEnvironment("RemoteApp__ApiKey", remoteApiKey) | ||
.WithReference(db, connectionName: "DefaultConnection") |
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.
Nice! One more tip as a NIT, if you want, you could probably use the client sql integration inside the framework app for connecting to the db, which would then light up all of the telemetry in the dashboard for all calls made to the db. Not needed of course, but just in case you want to try it out.
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 something I want to look into after this PR settles - it's a framework app using entity framework 6, so not sure how well that all integrates
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.
@twsouthwick I do have an EF6 sample for IIS with aspire and have just made sure it includes the SQL Server telemetry.
It also does all the stuff to dynamically connect using Aspire connection strings, and uses Microsoft.EntityFramework.SqlServer / Microsoft.Data.SqlClient.
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 looks awesome, thanks Taylor!
"environmentVariables": { | ||
"ASPNETCORE_ENVIRONMENT": "Development", | ||
"DOTNET_ENVIRONMENT": "Development", | ||
"DOTNET_DASHBOARD_OTLP_ENDPOINT_URL": "https://localhost:21002", |
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.
I think you need to use DOTNET_DASHBOARD_OTLP_HTTP_ENDPOINT_URL as framework does not support https over grpc.
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.
yup figured that out on my next PR :) I hadn't hooked up OTLP yet
This updates the samples to use Aspire for connecting the client ASP.NET Core and the server ASP.NET Framework applications. This does the following:
NOTE These are still not being built by CI - that is being covered by a separate PR at #566