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 <[email protected]>
- Remove legacy mocks - Update Makefile Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
… reports Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
|
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 <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
This is due to the fact that slog.DiscardHandler is not introduced until go 1.24 Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
This reverts commit ea29d0f. Signed-off-by: Jordan Blacker <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #354 +/- ##
==========================================
- Coverage 85.69% 80.79% -4.90%
==========================================
Files 20 27 +7
Lines 1433 2312 +879
==========================================
+ Hits 1228 1868 +640
- Misses 184 408 +224
- Partials 21 36 +15
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:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-structured multiprovider feature to the Go SDK. The implementation of different evaluation strategies, along with robust error and event handling, is a valuable addition. The code is clean, well-tested, and demonstrates good concurrency practices. My review includes a few minor suggestions to enhance maintainability and correct a documentation inconsistency.
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
|
I'm going to be away until Sept 2nd, so I likely won't have the time to review thoroughly until then. |
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
jblacker
force-pushed
the
implement-multiprovider
branch
from
304831b to
76cf8b7
Compare
|
|
||
| // NewComparisonStrategy creates a new instance of ComparisonStrategy. The fallback provider specified is called when | ||
| // there is a comparison failure -- prior to returning a default result. The Comparator parameter is optional and nil | ||
| // can be passed as long as ObjectEvaluation is never called. Unless the `alwaysUseCustom` parameter is true the default |
There was a problem hiding this comment.
I don't see an alwaysUseCustom parameter here. Go doc comments also don't do anything special with backticks, so you should avoid them.
There was a problem hiding this comment.
You're right. That was removed during some refactoring, but I forgot to update the docs. I'll get this cleaned up. The backticks is just a bad habit from markdown. I'll clean that up too throughout.
| // there is a comparison failure -- prior to returning a default result. The Comparator parameter is optional and nil | ||
| // can be passed as long as ObjectEvaluation is never called. Unless the `alwaysUseCustom` parameter is true the default | ||
| // comparisons built into Go will be used. The custom Comparator will only be used for ObjectEvaluation. However, if the | ||
| // parameter is set to true the custom Comparator will always be used regardless of evaluation type. If ObjectEvaluation |
There was a problem hiding this comment.
Please use doc links to refer to any exported identifier, like [Comparator] and [of.FeatureProvider.ObjectEvaluation]. See https://tip.golang.org/doc/comment#doclinks
There was a problem hiding this comment.
Updated throughout.
| // StrategyFirstMatch First provider whose response that is not FlagNotFound will be returned. This is executed | ||
| // sequentially, and not in parallel. | ||
| StrategyFirstMatch EvaluationStrategy = "strategy-first-match" | ||
| // StrategyFirstSuccess First provider response that is not an error will be returned. This is executed sequentially |
There was a problem hiding this comment.
| // StrategyFirstSuccess First provider response that is not an error will be returned. This is executed sequentially | |
| // StrategyFirstSuccess returns the first provider response that is not an error. This is executed sequentially. |
Write the other doc comments in a similar fashion. They should read like sentences and the first word after the identifier being documented should not be capitalized.
There was a problem hiding this comment.
Ack, I'll update throughout
| ReasonAggregated of.Reason = "AGGREGATED" | ||
| ReasonAggregatedFallback of.Reason = "AGGREGATED_FALLBACK" |
There was a problem hiding this comment.
This should be in its own const () block since it's not an [EvaluationStrategy].
| ReasonAggregated of.Reason = "AGGREGATED" | ||
| ReasonAggregatedFallback of.Reason = "AGGREGATED_FALLBACK" | ||
|
|
||
| ErrAggregationNotAllowedText = "object evaluation not allowed for non-comparable types without custom comparable func" |
There was a problem hiding this comment.
This should be in its own const () block since it's not an [EvaluationStrategy].
| // AsNamedProviderSlice Converts the map into a slice of NamedProvider instances | ||
| func toNamedProviderSlice(m ProviderMap) []*NamedProvider { |
There was a problem hiding this comment.
Update the doc comment to match the function name
|
Left some more comments. I didn't get time to look at the whole PR, so I may have more comments later. |
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Signed-off-by: Jordan Blacker <[email protected]>
Co-authored-by: Sahid Velji <[email protected]> Signed-off-by: Jordan Blacker <[email protected]>
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