feat: Implement multiprovider #354
Conversation
- Also updated the generated filenames to remove the test prefix allowing them to be exportable - Added a sed command to prepend the build tag "testtools" to the mock files - Updated test commands to include the "testtools" build tag - Added step to delete the backups generated by sed Signed-off-by: Jordan Blacker <jblacker@justworks.com>
- Remove legacy mocks - Update Makefile Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
… reports Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@users.noreply.github.com>
|
Hey @jblacker. Is there any road block to move this forward? Do you need any help? |
|
I've actually been on vacation. I'm going to pick back up on this tomorrow. |
Signed-off-by: Jordan Blacker <jblacker@users.noreply.github.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
This is due to the fact that slog.DiscardHandler is not introduced until go 1.24 Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
This reverts commit ea29d0f. Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #354 +/- ##
==========================================
- Coverage 85.89% 80.79% -5.11%
==========================================
Files 20 27 +7
Lines 1446 2327 +881
==========================================
+ Hits 1242 1880 +638
- Misses 183 409 +226
- Partials 21 38 +17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
- totalStatus -> overallStatus - totalStatusLock -> overallStatusLock - strategy -> strategyFunc Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Co-authored-by: Sahid Velji <sahidvelji@gmail.com> Signed-off-by: Jordan Blacker <jblacker@users.noreply.github.com>
Co-authored-by: Sahid Velji <sahidvelji@gmail.com> Signed-off-by: Jordan Blacker <jblacker@users.noreply.github.com>
|
I handled all of the additional comments. IMHO most of the latest comments are nits rather than true blockers, but I've handled them nonetheless. I like the idea of considering this experimental until it has some more usage under its belt by someone other than me. |
|
@jblacker Ok let's add a package doc comment stating that the package is experimental and that breaking changes are to be expected. Once added, I think we're good to merge. |
While reviewing the code to write additional documentation I noticed that there's a problem with the way custom strategies are implemented. There's no way for it to process multiple providers! All the other strategy functions are created using a closure to include the providers. This didn't exist for custom strategies so it would never actually have a provider to call! I corrected this by adding a new function type `StrategyConstructor` for custom strategies to use to be able to close over the providers. Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Include info & links to multiprovider and call out that it's experimental Signed-off-by: Jordan Blacker <jblacker@justworks.com>
Signed-off-by: Jordan Blacker <jblacker@justworks.com>
|
I want to call out that while I was writing additional documentation* I found a major bug in how the custom strategy is implemented. I realized that there's no way to provide the providers in this case since all of the other strategies have "constructors" to close over the provider slice. I added a new type * put together a full readme with a big warning that the multi package is experimental in addition to the package comment |
Signed-off-by: Jordan Blacker <jblacker@users.noreply.github.com>
|
Thanks for all the work here @jblacker! |
|
@jblacker Could you please look into the DCO issue? |
|
@erka The DCO issue is because I merged in your PR to my branch too quickly because you had two unsigned commits. @toddbaert said to not worry about it because he can squash them at merge time to eliminate that error here. |
|
Yeah, I can bypass the check. If we're ready, I can merge tomorrow. |
|
@beeme1mr I think we're good to merge, especially since we decided to mark the package experimental. |
Signed-off-by: Jordan Blacker <jblacker@users.noreply.github.com>
Head branch was pushed to by a user without write access
|
Just resolved the merge conflict that was pending. |
|
@beeme1mr please merge it |
7 participants
This PR
Implements the Multiprovider specified in "Appendix A: Included Utilities" of the OpenFeature Spec. This implementation is an improvement upon what I initially implemented in the
go-sdk-contribrepository here & here.This includes implementations for all three strategies described in the spec along with some common functions. Unlike my original implementation this includes full support for hooks & event handling from the included handlers.
Related Issues
open-feature/go-sdk-contrib#599
Notes
I reimplemented this here from the
go-sdk-contribrepo on suggestion of other members. This includes the refactoring improvements that I was mentioning in discussions on my open PR over there.This PR is currently in draft because it is built on top of the exported mocks in order to provide a full unit test suite.
Follow-up Tasks
How to test