feat: Add Unleash Provider #338
Conversation
|
nice 👍🏼 |
There was a problem hiding this comment.
Overl looks good from OF point of view. Please consider the review commits and related implementations.
@toddbaert hope you can have a look as well
providers/unleash/README.md
Outdated
| resolution := provider.BooleanEvaluation(ctx, "variant-flag", false, nil) | ||
| enabled := resolution.Value | ||
|
|
||
| resolution := provider.StringEvaluation(ctx, "variant-flag", "", nil) |
There was a problem hiding this comment.
| resolution := provider.StringEvaluation(ctx, "variant-flag", "", nil) | |
| resolution := provider.StringEvaluation(ctx, "variant-flag", "", nil) |
providers/unleash/pkg/provider.go
Outdated
|
|
||
| func NewProviderWithContext(ctx context.Context, providerConfig ProviderConfig) (*Provider, error) { | ||
| if providerConfig.Options != nil { | ||
| unleash.Initialize( |
There was a problem hiding this comment.
I think the lint is correct here. Initialize can fail, some of these are hard error cases like a broken URL, so we probably need to handle this somehow
providers/unleash/pkg/provider.go
Outdated
| } | ||
| } | ||
|
|
||
| feature := unleash.GetVariant(flag) |
There was a problem hiding this comment.
From a purely Unleash perspective, this is a variant, not a feature. If it makes more sense within this context to use feature, that's totally fine, but renaming this variable to variant makes a lot more sense to my eyes
providers/unleash/pkg/provider.go
Outdated
| } | ||
| } | ||
|
|
||
| feature := unleash.GetVariant(flag) |
There was a problem hiding this comment.
I don't think we're passing the Unleash context here, which is definitely needed for some toggles to successfully evaluate
providers/unleash/pkg/provider.go
Outdated
| } | ||
| } | ||
|
|
||
| res := unleash.IsEnabled(flag, unleash.WithFallback(defaultValue)) |
There was a problem hiding this comment.
Same as below, I think this needs to be passed the evalCtx parameter
| @@ -0,0 +1,99 @@ | |||
| package unleash_test | |||
There was a problem hiding this comment.
I'd really like to see some tests that check that the context passed in is respected. In the test data you have a "users-flag", which would need a context with userid on it to successfully return true. That might be a nice flag to build some tests off of for this
liran2000
force-pushed
the
feature/unleash
branch
from
6291f98
to
b9fc523
Compare
github-actions
bot
requested review from
bacherfl,
thomaspoignant and
toddbaert
liran2000
force-pushed
the
feature/unleash
branch
from
b9fc523
to
6700456
Compare
liran2000
force-pushed
the
feature/unleash
branch
from
6700456
to
32f3cd2
Compare
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
Signed-off-by: liran2000 <[email protected]>
|
@sighphyre @Kavindu-Dodan @toddbaert thank you for the valuable feedback, I addressed your comments. * had to rebase due to missing sign-off in some commits from a different IDE. |
| @@ -8,5 +8,6 @@ | |||
| "providers/go-feature-flag": "0.1.29", | |||
| "providers/flagsmith": "0.1.4", | |||
| "providers/launchdarkly": "0.1.3", | |||
| "tests/flagd": "1.3.1" | |||
| "tests/flagd": "1.3.1", | |||
| "tests/unleash": "0.0.1-alpha" | |||
There was a problem hiding this comment.
@liran2000 I guess this was a copy paste error, and you meant to do providers/unleash. I will force a quick fix for this, right now this is causing a 1.0 release which I don't think you want.
Sorry I missed this! 😅
There was a problem hiding this comment.
Fixed and release looks good now: #346
Add Unleash Provider.
See readme for details.
@sighphyre @gastonfournier @ivarconr you are welcome to review from Unleash perspective.
@Kavindu-Dodan @toddbaert you are welcome to review from OpenFeature perspective.