-
Notifications
You must be signed in to change notification settings - Fork 22
feat: Deprecate AddHostedFeatureLifecycle method #531
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
feat: Deprecate AddHostedFeatureLifecycle method #531
Conversation
|
@toddbaert This PR is to move the deprecated method into the |
|
@kylejuliandev I suggest moving all files currently in the Hosting package (under the Why this makes sense
Also, in the Hosting package, we currently have For example: services.ConfigureOptions<FeatureLifecycleStateOptions>(options =>
{
// Optional: configure StartState, StopState, etc.
});All of these options are optional, but this makes it easier and more consistent with typical .NET configuration. |
I am more inclined to move all The dependency tree generally looks like: We should keep only the DI unless we provide only DI extensions. Since we also offer application configuration, I vote to keep the We might need to make changes to the configuration file since I would like to keep the same namespaces we are using now.
Agreed! |
Signed-off-by: Kyle Julian <[email protected]>
* Update Samples App to show how you can work with OpenFeature and Dependency Injection Signed-off-by: Kyle Julian <[email protected]>
* Copy the existing Dependency Injection tests over to a new Hosting.Tests project * Fix issue with the Integration tests Signed-off-by: Kyle Julian <[email protected]>
2a50617 to
211e984
Compare
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #531 +/- ##
==========================================
+ Coverage 89.73% 90.04% +0.31%
==========================================
Files 64 77 +13
Lines 2523 2873 +350
Branches 295 323 +28
==========================================
+ Hits 2264 2587 +323
- Misses 199 226 +27
Partials 60 60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@kylejuliandev the code coverage went quite down. You did a copy-paste of the current code without changes? |
Yep, pretty much a copy and paste and update of namespaces and usings. Looking at the code coverage from main: We never produced a report for I was planning on adding some additional unit tests to improve the coverage - if that's ok? |
Go for it! 👍 |
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
beeme1mr
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.
As a follow up, we'll need to update the readme with the updated config. I believe @askpt updated the OTel demo to use DI, so we should help with with the migration once this has been released.
I'd like like to wait for @arttonoyan approval since he was the original author of the DI feature. Thanks!
Thanks for pointing out the main When the next version is released, I will update the Otel-demo repository 👍 |
|
/gemini review |
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.
Code Review
This pull request effectively refactors the dependency injection capabilities into the OpenFeature.Hosting package and deprecates the AddHostedFeatureLifecycle method. The new DI structure, centered around OpenFeatureBuilder, is well-designed and the extensive test suite is a valuable addition. I've identified a couple of areas for improvement to enhance the API's robustness by adding validation and providing safer default configurations.
Signed-off-by: Kyle Julian <[email protected]>
This PR
AddOpenFeatureThis is technically a breaking change because anyone using both packages in their project, which everyone will be, they will run into issues around resolving the correct extension methods.
Related Issues
Fixes #472
Notes
Follow-up Tasks
How to test