-
Notifications
You must be signed in to change notification settings - Fork 15
fix: lower the min version of system.* dependencies and cleaned up unnecessary ones #144
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
WalkthroughThe PR updates the project file to lower System.* package versions for non-.NET 8 targets (from 9.x to 8.x) and removes unnecessary dependencies like Microsoft.Bcl.AsyncInterfaces, addressing overly restrictive version requirements that caused compatibility issues in consuming applications. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
Pull Request Overview
This PR addresses dependency version constraints in the OpenFGA .NET SDK by lowering minimum required versions of System.* packages from 9.0.9 to 8.* and reorganizing package references to reduce unnecessary dependencies for consumers.
Key Changes:
- Reduced minimum versions of System.* dependencies from 9.0.9 to 8.0.x to broaden compatibility
- Removed unconditional dependencies that are only needed for pre-.NET 8 frameworks
- Consolidated package references under appropriate framework conditions
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/OpenFga.Sdk/OpenFga.Sdk.csproj(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-30T04:00:16.791Z
Learnt from: evansims
PR: openfga/dotnet-sdk#122
File: docs/Nodes.md:7-7
Timestamp: 2025-09-30T04:00:16.791Z
Learning: The docs/ directory in the openfga/dotnet-sdk repository contains auto-generated documentation files that should not be manually edited. Markdown formatting issues in these files should be disregarded during review.
Applied to files:
src/OpenFga.Sdk/OpenFga.Sdk.csproj
📚 Learning: 2025-09-30T04:00:25.010Z
Learnt from: evansims
PR: openfga/dotnet-sdk#122
File: docs/Users.md:7-7
Timestamp: 2025-09-30T04:00:25.010Z
Learning: The docs/ folder in the openfga/dotnet-sdk repository contains auto-generated API documentation from code comments. Markdown formatting issues in files under docs/ should be disregarded as they are generated and not manually edited.
Applied to files:
src/OpenFga.Sdk/OpenFga.Sdk.csproj
🔇 Additional comments (2)
src/OpenFga.Sdk/OpenFga.Sdk.csproj (2)
42-47: Dependency downgrade strategy verified—no breaking changes detected.Verification confirms the removal of
Microsoft.Extensions.HttpandMicrosoft.Bcl.AsyncInterfacesis safe:
- No usage of
IHttpClientBuilder,AddHttpClient,HttpClientFactory, or direct imports of either package- No async enumerable patterns requiring
Microsoft.Bcl.AsyncInterfaces- Version split (System.Text.Json 8.0.5 vs System.Net.Http.Json 8.0.1) is intentional and compatible
- Conditional structure correctly targets non-.NET 8 runtimes only
50-54: System.Text.Json 8.0.5 requires System.Memory (≥ 4.5.5) and System.Threading.Tasks.Extensions (≥ 4.5.4), both of which are satisfied by the net48-specific versions 4.6.3 declared in the project file. The package versions are compatible; no changes are needed.
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
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SoulPancake
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.
LG, Thanks @OsmanMElsayed
antonio-dfg
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.
🚀
rhamzeh
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.
Thanks @OsmanMElsayed
|
Errors like this should fail the build - opened #148 to track |
Gladly! And huge congrats to you guys for becoming a CNCF Incubating project 🎉! @rhamzeh |
Description
What problem is being solved?
#143
How is it being solved?
References
Review Checklist
mainSummary by CodeRabbit