-
Notifications
You must be signed in to change notification settings - Fork 45
feat: Implement multiprovider #354
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
base: main
Are you sure you want to change the base?
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:
|
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]>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack, I'll update throughout
ReasonAggregated of.Reason = "AGGREGATED" | ||
ReasonAggregatedFallback of.Reason = "AGGREGATED_FALLBACK" |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the doc comment to match the function name
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.
Fixed
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]>
Hey @open-feature/sdk-golang-maintainers can we get some reviews done here? |
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-contrib
repository 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-contrib
repo 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