feat: Add support to .NET v10
Add support for .NET V10
For now, I've removed code to support MySQL EF Core from the sample because it doesn't have support for .NET V10. Once this PR https://github.com/PomeloFoundation/Pomelo.EntityFrameworkCore.MySql/pull/2019 is merged and the package is generated, I'll undo my changes on the sample
For MongoDB EF Core I'll keep on .NET 8 & 9, waiting for https://jira.mongodb.org/projects/EF/issues/EF-272
Pull Request Review: .NET 10 Support
Thank you for this PR! Adding .NET 10 support is a significant undertaking. Here's my comprehensive review:
โ Positive Aspects
1. Comprehensive Scope
- All 80 changed files have been updated systematically
- Samples, tests, and source projects all migrated consistently
- CI/CD workflow properly updated for .NET 10 preview
2. Good Architectural Decisions
- Using
BrighterTargetFrameworkproperty to centralize target framework management is excellent - Conditional package references based on
TargetFrameworkmaintains backward compatibility - Multi-targeting approach (
netstandard2.0;net8.0;net9.0;net10.0) allows gradual adoption
3. Package Management
- Properly using centralized package management via
Directory.Packages.props - Preview packages (rc.2) are appropriately versioned for .NET 10
๐ Critical Issues
1. Typo in System.Text.Json Package References (Lines 138 & 159 in Directory.Packages.props)
<!-- CURRENT (INCORRECT) -->
<PackageVersion Include="System.Text.Json" ersion="9.0.9" />
<PackageVersion Include="System.Text.Json" ersion="10.0.0-rc.2.25502.107" />
<!-- SHOULD BE -->
<PackageVersion Include="System.Text.Json" Version="9.0.9" />
<PackageVersion Include="System.Text.Json" Version="10.0.0-rc.2.25502.107" />
Missing 'V' in 'Version' attribute - This will cause build failures.
Location: Directory.Packages.props:138 and Directory.Packages.props:159
โ ๏ธ Concerns & Questions
2. CI/CD Changes May Break Existing .NET 8/9 Testing
The workflow now only installs .NET 10:
dotnet-version: '10.x.x'
dotnet-quality: 'preview'
Problem: If the codebase still multi-targets net8.0 and net9.0 (which it does according to src/Directory.Build.props:32), you need those SDKs installed to build and test those target frameworks.
Recommendation: Either:
- Keep multi-SDK installation:
dotnet-version: | 8.0.x 9.0.x 10.x.x dotnet-quality: 'preview' - OR document that you're dropping .NET 8/9 from CI (though source still targets them)
3. Pomelo.EntityFrameworkCore.MySql Not Updated for .NET 10
Line 189 in Directory.Packages.props:
<PackageVersion Include="Pomelo.EntityFrameworkCore.MySql" Version="9.0.0" />
While other EF Core packages are using 10.0.0-rc.2, Pomelo is still at 9.0.0. This might cause compatibility issues if EF Core 10 has breaking changes.
Check: Does Pomelo 9.0.0 work with EF Core 10.0.0-rc.2? You may need to watch for a Pomelo preview release.
4. Microsoft.Extensions.TimeProvider.Testing Version Discrepancy
Line 158:
<PackageVersion Include="Microsoft.Extensions.TimeProvider.Testing" Version="9.9.0" />
This is using version 9.9.0 for .NET 10, while other Microsoft.Extensions.* packages are at 10.0.0-rc.2. Is there a reason, or is a 10.0.0 preview not available yet?
5. Assembly Version Updated to 10.0.0
src/Directory.Build.props:26-29 shows assembly versions updated to 10.0.0. This is a breaking semantic version change.
Questions:
- Is this intentional for .NET 10 support, or should this be a minor/patch version?
- Are there breaking API changes that warrant a major version bump?
- Should this be coordinated with the .NET 10 release timeline?
6. [WIP] Tag - What's Remaining?
The PR is marked as [WIP]. What tasks remain before this is ready for merge?
Suggested checklist:
- [ ] Fix System.Text.Json typos
- [ ] Verify all tests pass on .NET 10
- [ ] Resolve CI/CD multi-targeting concerns
- [ ] Verify third-party package compatibility (Pomelo, etc.)
- [ ] Update documentation/release notes
- [ ] Performance testing on .NET 10
๐ Best Practices Alignment
โ Follows Repository Guidelines:
- Central package management via
Directory.Packages.propsโ - Consistent target framework management โ
- All Microsoft.Extensions.* packages aligned within each TFM โ
โ ๏ธ Potential Concerns:
- Mixing preview (.NET 10 RC) with stable packages (some dependencies) - acceptable for preview work but note for production
- No test coverage changes visible - are there new tests for .NET 10-specific features?
๐ Security & Performance
- No security concerns identified - this is infrastructure/build configuration
- Performance considerations: .NET 10 typically brings performance improvements; no regressions expected
- Preview/RC quality means thorough testing needed before production use
๐ Recommendations
High Priority:
- Fix the System.Text.Json typos immediately - blocking issue
- Clarify CI/CD multi-targeting strategy - may cause build failures
- Verify Pomelo MySQL provider compatibility
Medium Priority:
- Document what's remaining for WIP โ ready
- Consider adding CI job that tests all target frameworks (not just net10.0)
- Update CHANGELOG/release notes for .NET 10 support
Nice to Have:
- Add smoke tests specifically targeting .NET 10 features if applicable
- Document minimum SDK version required for contributors
๐ฏ Summary
This is a well-structured PR for .NET 10 support with good architectural decisions. The main blockers are:
- Critical typo in package references (easy fix)
- CI/CD strategy needs clarification
Once these are addressed, the PR should be in good shape! The systematic approach to updating all projects is commendable.
Overall Assessment: โ ๏ธ Needs fixes before merge, but solid foundation
Let me know if you have questions about any of these points! Happy to discuss the multi-targeting strategy or help validate package compatibility.
Pull Request Review: .NET 10 Support
Overview
This PR adds support for .NET 10 to the Brighter project. The changes are systematic and well-structured, covering build configuration, dependency management, CI/CD pipeline updates, and project files across the entire codebase.
Positive Aspects
1. Systematic Approach
- Consistent pattern: All 78 .csproj files updated with
$(BrighterTargetFramework)variable - Centralized configuration: Smart use of
Directory.Build.propsto define the target framework globally - Multi-targeting support: The
src/Directory.Build.propsproperly defines multi-targeting withnet8.0;net9.0;net10.0
2. Dependency Management
- Framework-specific packages: Properly conditional package versions for .NET 8, 9, and 10
- Consistent version updates: All Microsoft.Extensions.* and System.* packages aligned to version 10.0.0 for .NET 10
- EF Core versioning: Correct separate versioning for EF Core packages across frameworks (8.0.22, 9.0.10, 10.0.0)
3. CI/CD Updates
- Simplified configuration: Moved from multi-version matrix to single .NET 10 preview
- Consistent quality setting: All jobs now use
dotnet-quality: 'preview' - Updated tooling: dotnet-ef updated to 10.0.0
Issues and Concerns
1. CRITICAL: Missing Pomelo.EntityFrameworkCore.MySql Package
The Pomelo.EntityFrameworkCore.MySql package has been removed from Directory.Packages.props but no explanation or migration path is provided:
- Lines removed: Original file had Pomelo package for .NET 8 and 9
- Impact: This will break MySQL/MariaDB Entity Framework Core support
- Action required:
- Either restore the package with .NET 10 compatible version
- Or document the migration strategy if MySQL support is being dropped
- Or add to known limitations if .NET 10 version isn't available yet
Location: Directory.Packages.props:165-172 (net8.0), 173-180 (net9.0)
2. Dependency Management Violation
According to docs/agent_instructions/dependency_management.md:33:
"Avoid mixing preview and stable package versions."
This PR mixes:
- Preview packages: All .NET 10 Microsoft packages at version 10.0.0 (preview)
- Stable packages: Third-party packages like MongoDB.Driver, RabbitMQ.Client, etc.
Recommendation: This is likely acceptable for a WIP preview support PR, but should be documented in the PR description. Consider adding a note about which packages are preview vs stable.
3. Pre-release Dependency
Npgsql is using a release candidate version:
<PackageVersion Include="Npgsql" Version="10.0.0-rc.1" />
Considerations:
- RC versions can have breaking changes before final release
- May need to monitor for Npgsql 10.0.0 stable release
- Consider adding a TODO or tracking issue
4. CI/CD Simplification May Be Too Aggressive
The CI changed from testing multiple .NET versions to only .NET 10:
# Before
dotnet-version: |
8.0.x
9.0.x
# After
dotnet-version: '10.x.x'
dotnet-quality: 'preview'
Concerns:
- No longer testing .NET 8 or .NET 9 compatibility
- Since the code still targets net8.0, net9.0, and net10.0, the CI should verify all targets
- Breaking changes in .NET 10 won't be caught for older frameworks
Recommendation: Consider either:
- Testing all three frameworks (8, 9, 10) in CI
- Or explicitly document that CI only validates latest framework
- Matrix strategy could test net10.0 primarily with periodic net8.0/9.0 checks
5. TUnit Package Removed
Directory.Packages.props previously included:
<PackageVersion Include="TUnit" Version="0.67.10" />
This has been removed without explanation:
- Is TUnit no longer needed?
- Was it experimental and never used?
- Should this be documented in the PR?
6. Whitespace Inconsistency
Minor formatting issue in CI file (.github/workflows/ci.yml:155):
--health-retries 5 # Extra trailing spaces
Changed to:
--health-retries 5 # Better, but still has trailing spaces
Not critical, but shows opportunity for cleanup.
Security Considerations
- โ No security concerns identified
- โ No credentials or secrets exposed
- โ Dependency versions are from official sources
Performance Considerations
- โ No performance impact from configuration changes
- โน๏ธ .NET 10 may have performance improvements, but that's framework-level
- โน๏ธ Preview builds may have different performance characteristics
Test Coverage
Major Concern: The PR title says [WIP] but there's no indication of:
- What specific .NET 10 features are being tested
- Whether existing tests pass on .NET 10
- Any new .NET 10-specific issues discovered
Recommendation: Before removing WIP status:
- Document test results across all platforms
- Add notes about any .NET 10-specific behavior changes
- Verify all test suites pass with .NET 10 SDK
Code Quality
- โ Follows the project's centralized package management pattern
- โ Consistent use of MSBuild properties
- โ No code logic changes (only configuration)
- โ ๏ธ Missing documentation of breaking changes or limitations
Recommendations
Must Fix Before Merge
- Restore or document Pomelo.EntityFrameworkCore.MySql removal
- Document preview package usage in PR description
- Address CI multi-targeting strategy
Should Consider
- Add a CHANGELOG or migration guide entry
- Document any known .NET 10 preview limitations
- Add tracking issue for Npgsql RC dependency
- Verify all samples build and run on .NET 10
- Consider a compatibility matrix in documentation
Nice to Have
- Clean up trailing whitespace in CI file
- Document TUnit package removal
- Add .NET 10 testing instructions to CONTRIBUTING.md
Conclusion
This is a solid foundation for .NET 10 support with systematic changes across the codebase. The main concerns are:
- The missing MySQL EF Core provider (critical)
- CI/CD strategy for multi-targeting
- Documentation of preview/RC dependencies
The structural approach is excellent and follows the project's patterns well. Once the critical issues are addressed and documentation is added, this will be ready for merge.
Overall Assessment: Good start, but needs attention to dependency completeness and CI strategy before removing WIP status.
Pull Request Review: Add .NET 10 Support
Thank you for working on adding .NET 10 support to Brighter! This is an important update. I've reviewed the changes and have some feedback organized by category.
๐ด Critical Issues
1. .NET 10 Package Availability
Location: Directory.Packages.props
The PR references .NET 10 packages, but as of January 2025, .NET 10 has not been released yet. The expected timeline is:
- .NET 10 Preview: Expected early 2025
- .NET 10 GA: Expected November 2025
Issue: Many packages in Directory.Packages.props reference version 10.0.0 which don't exist yet:
Microsoft.EntityFrameworkCore10.0.0Microsoft.Extensions.*10.0.0Npgsql10.0.0System.Text.Json10.0.0TickerQ10.0.0Microsoft.EntityFrameworkCore.MySql10.0.0
Impact: This PR will fail to build because these packages don't exist on NuGet yet.
Recommendation:
- Either wait until .NET 10 preview packages are available, or
- Use conditional package references that only apply when building against .NET 10 once it's available
- Monitor NuGet for package availability
โ ๏ธ Major Concerns
2. CI/CD Workflow Testing
Location: .github/workflows/ci.yml
The workflow adds 10.0.x to all test jobs, which is good for future-proofing. However:
Issue: The CI will fail immediately because:
- .NET 10 SDK isn't available yet for GitHub Actions
- The workflow doesn't have conditional logic to skip .NET 10 tests if the SDK isn't available
Recommendation:
- name: Setup .NET
uses: actions/setup-dotnet@v4
with:
dotnet-version: |
8.0.x
9.0.x
continue-on-error: true # Allow to continue if 10.0.x isn't available
Or use a matrix strategy with conditional execution for .NET 10.
3. Package Version Management Strategy
Location: Directory.Packages.props
The current approach uses framework-specific <ItemGroup> conditions, which is good. However:
Issues:
- The condition logic has a problem:
<ItemGroup Condition="'$(TargetFramework)' == 'net10.0' OR '$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'net462' OR '$(TargetFramework)' == 'net472'">
This groups .NET 10 with .NET Framework and netstandard2.0, which seems incorrect. .NET 10 should have its own section separate from legacy frameworks.
- Some packages may not support netstandard2.0 at version 10.0.0
Recommendation: Separate .NET 10 into its own conditional block:
<ItemGroup Condition="'$(TargetFramework)' == 'net10.0'">
<!-- .NET 10 specific packages -->
</ItemGroup>
4. Missing Package Version Updates
Location: Directory.Packages.props
While .NET 8 and 9 packages were updated to latest patch versions, several framework-agnostic packages should also be updated:
MySqlConnector: 2.4.0 โ 2.5.0 (good! โ)OpenTelemetry.*: Updated (good! โ)Microsoft.NET.Test.Sdk: 18.0.0 โ 18.0.1 (good! โ)
But consider also updating:
Microsoft.Data.SqlClient(currently 6.1.2)- Other non-framework-specific packages
๐ Minor Issues & Suggestions
5. Assembly Version Bumping
Location: src/Directory.Build.props
<AssemblyVersion>10.0.0.0</AssemblyVersion>
<FileVersion>10.0.0.0</FileVersion>
<InformationalVersion>10.0.0</InformationalVersion>
<Version>10.0.0</Version>
Issue: The assembly version was bumped to 10.0.0, but this may be premature if you're only adding .NET 10 support rather than making breaking changes.
Recommendation: Consider whether this warrants a major version bump according to your versioning policy. Adding framework support is typically a minor version change unless there are breaking API changes.
6. Documentation
Issue: No updates to documentation explaining .NET 10 support status, compatibility matrix, or migration guide.
Recommendation: Add or update:
- README.md with .NET 10 support status
- Release notes mentioning .NET 10 support
- Migration guide if there are any breaking changes
7. Removed Package References
Location: Various .csproj files
Some projects had direct package references removed (e.g., in Paramore.Brighter.Mediator.csproj, Paramore.Brighter.csproj). This appears to be consolidation into Directory.Packages.props.
Observation: This is good for centralized package management, but ensure all projects still compile correctly with transitive dependencies.
8. Whitespace Changes
Location: .github/workflows/ci.yml:160
Minor formatting change (trailing space removed). Good cleanup! โ
โ Positive Aspects
- Consistent approach: All sample projects updated consistently with variable reference
- Centralized versioning: Good use of
Directory.Packages.propsfor version management - Framework-specific packages: Proper separation of package versions by target framework
- CI coverage: .NET 10 added to all test jobs for comprehensive testing
- Tool updates: dotnet-ef updated to 10.0.0
- Multi-targeting: Proper support for net8.0, net9.0, and net10.0 simultaneously
๐ง Code Quality & Best Practices
According to the repository's coding standards (from CLAUDE.md):
โ Follows conventions:
- Proper use of MSBuild properties
- Consistent file structure
- Clear separation of concerns in package management
โ ๏ธ Areas for improvement:
- Testing guidelines suggest comprehensive test coverage - ensure .NET 10-specific tests are added once SDK is available
- Documentation standards require updates to reflect new framework support
๐ก๏ธ Security Considerations
No security concerns identified. The changes are primarily configuration updates for framework support.
๐ฏ Performance Considerations
.NET 10 is expected to bring performance improvements. Once released:
- Monitor for JIT improvements
- Take advantage of new runtime features
- Review any deprecated APIs
๐ Action Items
Before merging:
- โ ๏ธ BLOCKER: Wait for .NET 10 preview packages to be available on NuGet
- โ ๏ธ BLOCKER: Verify the PR builds successfully with .NET 10 SDK
- Update conditional logic in
Directory.Packages.propsto separate .NET 10 from legacy frameworks - Add documentation about .NET 10 support status
- Consider whether assembly version should be 10.0.0 or follow existing versioning scheme
- Add release notes entry
- Run full test suite against .NET 10 once SDK is available
Recommended approach: Given that .NET 10 hasn't been released yet, consider:
- Creating a feature branch that tracks .NET 10 preview releases
- Updating as preview packages become available
- Merging after .NET 10 GA and thorough testing
๐ Summary
Overall Assessment: This PR demonstrates good preparation for .NET 10 support with a well-structured approach to multi-targeting. However, it's premature as .NET 10 hasn't been released yet. The PR cannot be merged in its current state due to missing packages.
Recommendation:
- Mark as Draft/WIP (which it already is โ)
- Monitor .NET 10 preview releases
- Update incrementally as packages become available
- Plan for thorough testing once SDK is available
Risk Level: Low (once packages are available) Code Quality: Good Architecture: Sound approach to multi-targeting
Great work on being proactive with .NET 10 support! Once the preview packages are available, this will be ready to proceed with testing. ๐
Pull Request Review: .NET 10 Support
This is a substantial infrastructure update. Below is my detailed review.
Critical Issues
-
Incorrect Package Version Conditionals (Directory.Packages.props:182-214) - The conditional groups net10.0 with legacy frameworks (netstandard2.0, net462, net472), causing legacy frameworks to incorrectly receive .NET 10 package versions which will cause build failures. Split into separate conditions.
-
Non-Existent Package Versions - Several packages reference version 10.0.0 which do not exist yet: Microsoft.EntityFrameworkCore.MySql and Npgsql version 10.0.0 do not exist.
-
Hardcoded NotSupported Blocks - Startup.cs:156 and Program.cs:177 throw NotSupportedException for MySQL which breaks functionality.
Major Concerns
- Missing Test Coverage - No verification that tests pass on .NET 10
- Version Number Inconsistency - Assembly version bumped to 10.0.0 - is this intentional for just framework support?
- Incomplete EntityFrameworkCore Migration - No migration path for MySQL documented
Action Items (Must Fix Before Merge)
- Fix conditional logic in Directory.Packages.props (critical)
- Verify all package versions exist
- Fix or remove MySql NotSupported blocks
- Run complete test suite on .NET 10
- Decide on assembly version strategy
Summary
Excellent approach using centralized package management. However, critical package versioning issues must be fixed before merge. The conditional logic error will cause build failures on legacy frameworks.
Recommendation: Request Changes
@lillo42 Is this ready for review?
@iancooper it's ready, I'll resolve the conflicts
Pull Request Review: .NET 10 Support
Thank you for this comprehensive update to add .NET 10 support! This is a substantial PR that touches 88 files with well-organized changes. Here's my detailed review:
โ Strengths
-
Comprehensive Coverage: The PR systematically updates all samples and test projects across the entire codebase.
-
Centralized Configuration: Using BrighterTargetFramework property in Directory.Build.props is an excellent approach for managing framework versions centrally.
-
Framework-Specific Package Management: The conditional package versioning in Directory.Packages.props is well-structured, properly handling different package versions for .NET 8, 9, and 10.
-
CI/CD Updates: All GitHub Actions workflows properly include .NET 10 in the matrix builds.
-
Transparent Communication: Clear documentation in the PR description about the temporary MySQL EF Core limitation.
๐ Issues & Concerns
1. Package Version Inconsistencies (Critical)
In Directory.Packages.props, there's a problematic condition on line 163 that groups .NET 10 with .NET Framework 4.6.2, 4.7.2, and .NET Standard 2.0, meaning these older frameworks will attempt to use .NET 10 packages (version 10.0.0), which will fail.
Impact: This will break builds for projects targeting .NET Framework or .NET Standard 2.0.
Fix: The .NET 10 condition should be separate from legacy framework conditions.
2. Missing MongoDB.EntityFrameworkCore for .NET 10
In Directory.Packages.props, the .NET 10 section is missing MongoDB.EntityFrameworkCore (present in .NET 8 and 9 sections). If this package is needed, it should be included.
3. Version Downgrade
Swashbuckle.AspNetCore was downgraded from 10.0.1 to 9.0.6. If this is intentional due to compatibility issues with .NET 10, it should be documented. Otherwise, consider keeping the latest version.
4. Package Version Inconsistency - TickerQ
The package TickerQ appears with version 2.5.3 in the general section, but this seems inconsistent with the framework-specific sections. Verify which version should be used for .NET 10.
5. Sample Code - NotSupportedException Handling
In samples/WebAPI/WebAPI_EFCore/GreetingsWeb/Startup.cs:156 and SalutationAnalytics/Program.cs:177, throwing NotSupportedException in production code paths could cause runtime failures. Consider adding guard conditions earlier in the configuration and using more specific exception messages.
๐งช Testing Considerations
-
Test Coverage: No new tests were added, but this is acceptable for a framework upgrade. Existing tests should validate the migration.
-
CI Matrix: Excellent that all test jobs now include .NET 10 in the matrix.
-
Verification Needed:
- Ensure all test suites pass with .NET 10
- Verify package restore works correctly for all target frameworks
- Test that .NET Framework and .NET Standard 2.0 projects still build
๐ Minor Observations
-
Npgsql Version: For .NET 9, Npgsql was upgraded from 8.0.7 to 9.0.4. This is fine but worth noting.
-
OpenTelemetry.Exporter.Jaeger: Updated to 1.6.0-rc.1 (pre-release). Verify this doesn't introduce stability issues.
-
Project Files: The consistent use of BrighterTargetFramework across all samples is excellent and follows the DRY principle.
๐ฏ Recommendations
-
CRITICAL: Fix the conditional package version grouping in Directory.Packages.props line 163.
-
HIGH: Add missing MongoDB.EntityFrameworkCore package for .NET 10 if needed.
-
MEDIUM: Document or justify the Swashbuckle downgrade.
-
MEDIUM: Review exception handling approach for MySQL EF Core limitation.
-
LOW: Consider separating the whitespace cleanup into a separate commit per Tidy First guidelines.
โ Security & Performance
- No security concerns identified
- No performance regressions expected
- Package updates appear appropriate
๐ Summary
This is a well-structured PR with comprehensive coverage of the .NET 10 migration. The main blocker is the package version condition issue that could break legacy framework builds. Once that is resolved, this should be good to merge (pending successful CI test runs).
Recommendation: Request changes to fix the critical package versioning issue, then approve after verification.
Great work on this migration! Let me know if you would like me to clarify any of these points.
Pull Request Review: Add support to .NET v10
Summary
This PR successfully adds .NET 10.0 support to the Brighter project. The changes are well-structured and comprehensive, updating CI workflows, package dependencies, and sample projects.
โ Strengths
-
Comprehensive Dependency Management: The
Directory.Packages.propsrefactoring is excellent - moving from global version specifications to framework-specific conditional groups makes dependency management much cleaner and maintainable. -
CI/CD Coverage: All test workflows in
.github/workflows/ci.ymlhave been updated to include .NET 10.0, ensuring consistent test coverage across frameworks. -
Centralized Configuration: Introduction of
BrighterTargetFrameworkproperty inDirectory.Build.propsprovides a single point of control for all sample projects, reducing duplication. -
Pragmatic Approach: Temporarily disabling MySQL EF Core support with clear
NotSupportedExceptionmessages and comments is the right approach while waiting for upstream dependencies.
๐ Code Quality Observations
Good Practices Observed:
- Clear exception messages explaining why operations are not supported
- Consistent use of the centralized
$(BrighterTargetFramework)property across all sample projects - Framework-specific package versioning properly organized
Areas for Consideration:
-
Package Versioning Strategy (samples/WebAPI/WebAPI_EFCore/Startup.cs:156 and SalutationAnalytics/Program.cs:177):
- The temporary MySQL EF Core workaround uses
NotSupportedExceptionwith a message about waiting for Pomelo support - Consider: This is appropriate for samples, but ensure this is documented in release notes so users know about this limitation
- The commented-out code should ideally be tracked in an issue and removed once support is restored
- The temporary MySQL EF Core workaround uses
-
Conditional Compilation (Directory.Packages.props:125):
- The condition
'$(TargetFramework)' == 'net10.0' OR '$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'net462' OR '$(TargetFramework)' == 'net472'groups .NET 10 with legacy frameworks - Rationale: This appears intentional to use the latest package versions for .NET 10 and legacy frameworks, but it's somewhat counterintuitive
- Consider: Adding a comment explaining why net10.0 is grouped with legacy frameworks (presumably because they all want the latest versions)
- The condition
-
Version Consistency:
- OpenTelemetry.Exporter.Jaeger upgraded to
1.6.0-rc.1(a pre-release version) - Consider: Document why a pre-release version is necessary (likely for .NET 10 compatibility)
- OpenTelemetry.Exporter.Jaeger upgraded to
-
Missing Package in Migration:
Microsoft.EntityFrameworkCore.MySqlis added to the net10.0 condition with version10.0.0- This appears premature since the code explicitly throws
NotSupportedExceptionfor MySQL - Consider: Remove this package reference until the underlying Pomelo package is ready
๐ก๏ธ Security & Performance
No Security Concerns Identified: The changes are primarily infrastructure and dependency updates with no security implications.
No Performance Concerns: The package updates should maintain or improve performance as they include latest optimizations from .NET 10.
๐ Test Coverage
Test Coverage Adequate:
- All 15 test workflow jobs updated to include .NET 10.0
- Tests will run against all supported frameworks (8.0, 9.0, 10.0)
Consider: Once MySQL support is restored, ensure integration tests cover all database types with .NET 10.
๐ Potential Issues
-
Minor: The
Microsoft.EntityFrameworkCore.MySqlpackage version10.0.0in Directory.Packages.props may not exist yet, which could cause restore failures if someone tries to use MySQL- Impact: Low - code will throw NotSupportedException anyway
- Suggestion: Either remove the package reference or add a comment that it's a placeholder
-
Whitespace Change (.github/workflows/ci.yml:178): There's a trailing whitespace cleanup (purely cosmetic)
๐ Suggestions
-
Documentation: Add a comment in
Directory.Packages.propsexplaining the framework grouping logic<!-- .NET 10.0 is grouped with legacy frameworks to use latest package versions --> <ItemGroup Condition="'$(TargetFramework)' == 'net10.0' OR '$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'net462' OR '$(TargetFramework)' == 'net472'"> -
Tracking: Create issues to track:
- Re-enabling MySQL EF Core support when Pomelo package is released
- Upgrading OpenTelemetry.Exporter.Jaeger from RC to stable release
-
PR Description: The PR body mentions the Pomelo PR link - consider adding an issue reference in the code comments for easy tracking
โจ Overall Assessment
Status: โ Approve with minor suggestions
This is a solid PR that successfully adds .NET 10.0 support in a well-organized manner. The dependency management refactoring improves code organization significantly. The temporary MySQL limitation is handled appropriately with clear error messages.
The suggestions above are minor improvements and don't block merging. Great work on maintaining consistency across all sample projects and test suites!
Code adheres to Brighter conventions: โ
Tests updated appropriately: โ
Breaking changes: None
Documentation needed: Release notes should mention MySQL EF Core limitation