-
Notifications
You must be signed in to change notification settings - Fork 0
feat: enhance provider with Bucketeer SDK integration and evaluation methods #3
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
Conversation
…methods
d70595e
to
1957b54
Compare
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.
Pull Request Overview
This PR integrates the Bucketeer SDK into the feature provider to support evaluation methods for Boolean, String, Float, Int, and Object flag types and maps Bucketeer evaluation reasons to OpenFeature reasons.
- Implements the BucketeerSDK interface with evaluation methods for various data types.
- Converts OpenFeature context into a Bucketeer user and maps evaluation reasons accordingly.
- Adds comprehensive unit tests for the new functionality.
Files not reviewed (1)
- go.mod: Language not supported
Comments suppressed due to low confidence (1)
pkg/provider.go:120
- The variable 'flag' appears to be used without a declaration in this function; please ensure it is passed as a parameter or defined within the scope to prevent undefined variable errors.
evaluation := p.sdk.BoolVariationDetails(ctx, bucketeerUser, flag, defaultValue)
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
Hi @nnnkkk7,
Great work!
I have a few comments—please take a look.
assert.Equal(t, test.expectedReason, actual) | ||
}) | ||
} | ||
} |
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.
Will you add the e2e tests ?
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.
Yes!
I'll add it in another PR!
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.
Yes, please add the document on how to initiate and use the provider, too.
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.
ok!
0de5986
to
0293115
Compare
0293115
to
39273ad
Compare
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.
Your changes look good.
I’m just wondering if we could support more data types in the OpenFeature EvaluationContext.
pkg/provider.go
Outdated
valStr, ok := val.(string) | ||
if !ok { | ||
return user.User{}, | ||
ToPtr(openfeature.NewParseErrorResolutionError( | ||
fmt.Sprintf("key %q, value %q can not be converted to string", key, val), | ||
), | ||
) | ||
} | ||
bucketeerUser.Data[key] = valStr |
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.
valStr, ok := val.(string) | |
if !ok { | |
return user.User{}, | |
ToPtr(openfeature.NewParseErrorResolutionError( | |
fmt.Sprintf("key %q, value %q can not be converted to string", key, val), | |
), | |
) | |
} | |
bucketeerUser.Data[key] = valStr | |
switch v := val.(type) { | |
case string: | |
bucketeerUser.Data[key] = v | |
default: | |
jsonBytes, err := json.Marshal(v) | |
if err != nil { | |
return user.User{}, ToPtr(openfeature.NewParseErrorResolutionError( | |
fmt.Sprintf("key %q, value %q cannot be converted to JSON string: %v", key, val, err), | |
)) | |
} | |
bucketeerUser.Data[key] = string(jsonBytes) | |
} |
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.
What do you think ?
If the val
contains unsupported data types (e.g., functions, channels, circular references), json.Marshal will fail and return an error.
Otherwise, it seems good.
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.
Thank you!
I fixed and added the test!
fix: add other data types to toBucketeerUser
assert.Equal(t, test.expectedReason, actual) | ||
}) | ||
} | ||
} |
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.
Yes, please add the document on how to initiate and use the provider, too.
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.
LGTM 👍
Uh oh!
There was an error while loading. Please reload this page.