-
Notifications
You must be signed in to change notification settings - Fork 22
feat: add AddHandler extension method to Dependency Injection package #462
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: add AddHandler extension method to Dependency Injection package #462
Conversation
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #462 +/- ##
==========================================
+ Coverage 86.72% 86.87% +0.14%
==========================================
Files 42 43 +1
Lines 1703 1722 +19
Branches 179 180 +1
==========================================
+ Hits 1477 1496 +19
Misses 187 187
Partials 39 39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Looks good to me. I just had a question about the random name generator but it isn't a blocker.
src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
askpt
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 to me. Great work. Added a couple of suggestions 👍
src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
* Add check if passed in handler name is an empty or null string Signed-off-by: Kyle Julian <[email protected]>
askpt
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.
It is a very minor request! We should also validate for inputs like whitespace.
Also, @arttonoyan, would you mind double-checking?
src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
test/OpenFeature.DependencyInjection.Tests/OpenFeatureBuilderExtensionsTests.cs
Outdated
Show resolved
Hide resolved
* Ensure that calling AddHandler multiple times will actually add all instances to the underlying OpenFeature API * Add unit tests to cover this behaviour Signed-off-by: Kyle Julian <[email protected]>
…-via-dependencyinjection Signed-off-by: Kyle Julian <[email protected]>
askpt
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.
LGTM. Just a minor question, and should be ready to merge! Thanks!
src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
src/OpenFeature.DependencyInjection/Internal/EventHandlerDelegateWrapper.cs
Outdated
Show resolved
Hide resolved
src/OpenFeature.DependencyInjection/OpenFeatureBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
* Throw ArgumentException when HandlerName is null or empty * Use primary constructor in EventHandlerDelegateWrapper Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
|
LGTM! If @arttonoyan is happy with the changes @kylejuliandev made, we can merge it 👍 |
src/OpenFeature.DependencyInjection/Internal/FeatureLifecycleManager.cs
Outdated
Show resolved
Hide resolved
* Ensure when multiple handlers are registered that they are both registered * Tweak CreateServerAsync to make it easier to extend dependency injection tests Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
* Remove any unnecessary unit tests * Update integration tests to use openFeatureBuilder for easily adjusting the Arrange part of each test Signed-off-by: Kyle Julian <[email protected]>
|
Thank you @askpt @arttonoyan @beeme1mr! This ended up being a code review and API review all in one; I appreciate the collaboration! |
…open-feature#462) <!-- Please use this template for your pull request. --> <!-- Please use the sections that you need and delete other sections --> ## This PR <!-- add the description of the PR here --> - Introduces a new extension method in the OpenFeature.DependencyInjection package which allows consumers to add, at a global/api level, a handler. Example usage: ```csharp builder.Services.AddOpenFeature(feature => { feature.AddHostedFeatureLifecycle() .AddInMemoryProvider() .AddHandler(ProviderEventTypes.ProviderReady, (@event) => { Console.WriteLine("{0}", @event!.ProviderName); }) .AddHandler(ProviderEventTypes.ProviderReady, sp => (@event) => { var logger = sp.GetRequiredService<ILogger<Program>>(); logger.LogInformation("Provider Ready"); }); }); ``` ### Related Issues <!-- add here the GitHub issue that this PR resolves if applicable --> Fixes open-feature#457 ### Notes <!-- any additional notes for this PR --> ### Follow-up Tasks <!-- anything that is related to this PR but not done here should be noted under this section --> <!-- if there is a need for a new issue, please link it here --> ### How to test <!-- if applicable, add testing instructions under this section --> --------- Signed-off-by: Kyle Julian <[email protected]> Signed-off-by: Weihan Li <[email protected]>
🤖 I have created a release *beep* *boop* --- ## [2.6.0](v2.5.0...v2.6.0) (2025-05-23) ### ✨ New Features * add AddHandler extension method to Dependency Injection package ([#462](#462)) ([ff414b8](ff414b8)) * Add Extension Method for adding global Hook via DependencyInjection ([#459](#459)) ([9b04485](9b04485)) * Add OTEL compatible telemetry object builder ([#397](#397)) ([6c44db9](6c44db9)) ### 🧹 Chore * Cleanup .props file ([#476](#476)) ([6d7a535](6d7a535)) * **deps:** update actions/attest-build-provenance action to v2.3.0 ([#464](#464)) ([0a5ab0c](0a5ab0c)) * **deps:** update codecov/codecov-action action to v5.4.3 ([#475](#475)) ([fbcf3a4](fbcf3a4)) * **deps:** update github/codeql-action digest to 60168ef ([#463](#463)) ([ea76351](ea76351)) * **deps:** update github/codeql-action digest to ff0a06e ([#473](#473)) ([af1b20f](af1b20f)) * **deps:** update spec digest to edf0deb ([#474](#474)) ([fc3bdfe](fc3bdfe)) ### 📚 Documentation * Add AspNetCore sample app ([#477](#477)) ([9742a0d](9742a0d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Signed-off-by: OpenFeature Bot <[email protected]> Signed-off-by: André Silva <[email protected]> Co-authored-by: André Silva <[email protected]>
This PR
Example usage:
Related Issues
Fixes #457
Notes
Follow-up Tasks
How to test