-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Multi provider impl #168
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
return eval | ||
} | ||
// Continue to next provider if error is FLAG_NOT_FOUND | ||
} catch (_: OpenFeatureError.FlagNotFoundError) { |
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.
No logging tool, just swallowing for the moment
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
79a2d9a
to
2142879
Compare
// Event precedence (highest to lowest priority) - based on the specifications | ||
private val eventPrecedence = mapOf( | ||
OpenFeatureProviderEvents.ProviderError::class to 4, // FATAL/ERROR | ||
OpenFeatureProviderEvents.ProviderNotReady::class to 3, // NOT READY, Deprecated but still supporting |
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.
Do we want to add back support for this Event?
According to the documentation https://openfeature.dev/specification/appendix-a/#status-and-event-handling
Initially the status is NOT_READY
The only way to represent the NOT_READY state is by setting the initial value of the eventFlow
to an Error, where, it would probably be smarter not to represent the initial state as being in "error"?
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 think we should look into this. I've thought about it recently as well and for me NOT_READY
is a valid state. I don't remember why it was deprecated.
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.
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.
Ah okay, was a bit confusing but Events rightfully don't have a NotReady state: https://openfeature.dev/specification/types#provider-events
The documentation makes it confusing by mentioning both Events and Statuses in the section (i.e ConfigurationChangedEvent)
Still trying to wrap my head around the specifications outlined here and what they actually mean lol: https://openfeature.dev/specification/appendix-a#multi-provider-status
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.
Okay, so looks like what we need to do is
- Listen to all events of the Providers and map those to STATUS emitted by the MultiProvider
- Listen to all events of the Providers and re-emit the event from our own
observe()
function if that particular event updates the STATUS of our MultiProvider. (Caveat here being that we always re-emit Configuration changed..)
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.
Okay, 4fa33eb this should now align with the specs.
Definitely need to do another round of double checking things
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/MultiProvider.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: penguindan <[email protected]>
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/MultiProvider.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/MultiProvider.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/FirstMatchStrategy.kt
Outdated
Show resolved
Hide resolved
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
* See: https://openfeature.dev/specification/appendix-a/#metadata | ||
*/ | ||
val originalMetadata: Map<String, ProviderMetadata> | ||
get() = emptyMap() |
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.
nice with defaults to not make it breaking 👍
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
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.
Thanks a lot for the contributions, @PenguinDan, they look very promising! Nothing serious, only minor findings from my side!
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/MultiProvider.kt
Show resolved
Hide resolved
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/MultiProvider.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/MultiProvider.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/MultiProvider.kt
Show resolved
Hide resolved
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/MultiProvider.kt
Outdated
Show resolved
Hide resolved
...dk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/FirstSuccessfulStrategy.kt
Show resolved
Hide resolved
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/Strategy.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/Strategy.kt
Outdated
Show resolved
Hide resolved
...dk/src/commonTest/kotlin/dev/openfeature/kotlin/sdk/multiprovider/FirstMatchStrategyTests.kt
Outdated
Show resolved
Hide resolved
sampleapp/src/main/kotlin/dev/openfeature/kotlin/sdk/sampleapp/ExampleProvider.kt
Outdated
Show resolved
Hide resolved
…rategy into MultiProvider Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
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.
Thanks for the updates, going to the right direction!
...dk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/FirstSuccessfulStrategy.kt
Outdated
Show resolved
Hide resolved
...dk/src/commonMain/kotlin/dev/openfeature/kotlin/sdk/multiprovider/FirstSuccessfulStrategy.kt
Outdated
Show resolved
Hide resolved
…ltiprovider/FirstSuccessfulStrategy.kt Co-authored-by: Bence Hornák <[email protected]> Signed-off-by: Daniel Kim <[email protected]>
Signed-off-by: penguindan <[email protected]>
Signed-off-by: penguindan <[email protected]>
This PR
Multi Provider implementation according to the specifications outlined in https://openfeature.dev/specification/appendix-a/#multi-provider
Related Issues
Fixes #149
Notes
Work that still needs to be done
Follow-up Tasks
How to test
Updated the sample application to utilize a multi provider