-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Upgrading eshop to .NET 10 and Aspire 9.4.2 #890
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
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 upgrades the eShop application from .NET 9 to .NET 10 and updates Aspire to version 9.4.2, along with updating various package dependencies.
- Updates all projects' target framework from net9.0 to net10.0
- Upgrades Aspire from 9.4.0 to 9.4.2 and updates related package versions
- Modernizes test assertions from Assert.ThrowsException to Assert.ThrowsExactly
- Updates OpenAPI handling to support OpenAPI 3.1 specification changes
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| global.json | Updates .NET SDK version to 10.0.100-preview.7.25380.108 |
| Directory.Packages.props | Updates package versions for .NET 10 compatibility including ASP.NET Core, Entity Framework, and testing frameworks |
| src/eShop.ServiceDefaults/OpenApiOptionsExtensions.cs | Modernizes OpenAPI implementation for 3.1 specification compatibility |
| Multiple .csproj files | Updates TargetFramework from net9.0 to net10.0 across all projects |
| Test files | Replaces Assert.ThrowsException with Assert.ThrowsExactly for more precise exception testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
…idation - Remove deprecated FluentValidation.AspNetCore package - Add FluentValidation and FluentValidation.DependencyInjectionExtensions packages - Update ValidatorBehavior to use async validation (ValidateAsync) - Replace manual validator registrations with assembly scanning - Maintain existing validation functionality and error handling Resolves deprecated package warnings while following official FluentValidation guidance for manual validation approach.
|
This should be now ready to go. Would appreciate any reviews to be able to unblock other folks wanting to make .NET 10 changes here. |
mikekistler
left a comment
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.
Looks good! 👍
I left a couple comments on things that looked odd but nothing serious or obviously wrong that should hold up merge.
| "description": "The catalog item id", | ||
| "required": true, | ||
| "schema": { | ||
| "pattern": "^-?(?:0|[1-9]\\d*)$", |
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 odd. "pattern" only applies to type: string but the id is type: integer.
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.
mm these changes were done by Copilot automatically. How can I regenerate this file to make sure it looks right?
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.
looks like I had pending changes for this, I just pushed the latest generated files based on the latest build
| { | ||
| var uri = $"{remoteServiceBaseUrl}items/{id}?api-version=2.0"; | ||
| return httpClient.GetFromJsonAsync<CatalogItem>(uri); | ||
| return httpClient.GetFromJsonAsync(uri, CatalogJsonContext.Default.CatalogItem); |
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 another change that looks wonky. It's probably right but I certainly wouldn't know that I needed to do this to migrate to .NET 10.
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.
You don't need to do this to migrate. This was to address some new trimmer warnings. Basically in .NET 10 new APIs were annotated for trimming warnings, and the way we had the code before made it so the trimmer wouldn't know what needs to be kept, so warnings were logged. This change makes it so that the trimmer understands which types need to be kept, so it fixes the warnings.
This reverts commit 318e037.
No description provided.