-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add flagsmith provider #68
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
feat: add flagsmith provider #68
Conversation
9127447 to
1ba0ba8
Compare
Summary of ChangesHello @Zaimwa9, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers a new OpenFeature provider for the Flagsmith Ruby SDK. Its primary purpose is to allow Ruby applications to seamlessly integrate with Flagsmith for feature flag management, adhering to the OpenFeature specification. The implementation covers all standard flag types, handles evaluation contexts for user-specific targeting, and incorporates comprehensive error handling and type validation. The PR is accompanied by extensive documentation, including design specifications and usage examples, ensuring clarity and ease of adoption. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces a new Flagsmith provider for the OpenFeature Ruby SDK. The implementation is comprehensive, including support for all flag types, context mapping, and thorough test coverage. The code is well-structured, with clear separation of concerns for options, errors, and the provider logic itself. I've identified a couple of areas for improvement: one is a potential bug in how evaluation context traits are handled, which could lead to incorrect flag evaluations. The other is an inconsistency between the implementation and the design documents regarding the resolution reason for local evaluation mode. Overall, this is a great contribution.
providers/openfeature-flagsmith-provider/lib/openfeature/flagsmith/provider.rb
Outdated
Show resolved
Hide resolved
providers/openfeature-flagsmith-provider/lib/openfeature/flagsmith/provider.rb
Show resolved
Hide resolved
|
@gemini-code-assist, please review again |
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.
Code Review
This pull request introduces a new Flagsmith provider for the OpenFeature Ruby SDK. The implementation is comprehensive and includes good test coverage. I've found a few areas for improvement:
- A critical bug in numeric value parsing that causes errors for float-like strings.
- An issue where disabled flags are incorrectly reported as not-found. I've suggested a refactoring to correctly handle the
DISABLEDreason. - A minor documentation update in the README to include the
DISABLEDreason.
Overall, this is a great contribution. Addressing these points will make the provider more robust and compliant with the OpenFeature specification.
providers/openfeature-flagsmith-provider/lib/openfeature/flagsmith/provider.rb
Show resolved
Hide resolved
providers/openfeature-flagsmith-provider/lib/openfeature/flagsmith/provider.rb
Outdated
Show resolved
Hide resolved
|
@gemini-code-assist, do you find any critical or high priority issues ? |
|
Hello @Zaimwa9, thanks for asking for a follow-up! I've re-reviewed the pull request for critical and high-priority issues. Overall, the implementation looks solid and well-structured, adhering to the OpenFeature provider patterns. I did not identify any critical issues that would immediately break functionality or introduce severe security flaws. However, I did find one area that could become a high-priority concern depending on the scale of usage, and another that is a medium-priority maintainability concern: High Priority (Potential Efficiency Issue)
Medium Priority (Maintainability Issue)
I recommend addressing the flag lookup efficiency first, especially if you anticipate a large number of feature flags in your Flagsmith environments. The broad exception handling is also important for long-term maintainability and easier debugging. |
|
Following latest comment:
|
|
Taking a look now! |
toddbaert
left a comment
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.
@Zaimwa9 Ruby is not my strong suit, but I've reviewed to the best of my ability according to the OpenFeature specification, and this seems good to me.
Before we merge, could you rebase/merge main and add an entry with this component and your name here? This just auto-adds you to PRs for this component.
🤖 I have created a release *beep* *boop* --- ## [0.0.5](open-feature/ruby-sdk-contrib@openfeature-meta_provider/v0.0.4...openfeature-meta_provider/v0.0.5) (2025-03-03) ### 🐛 Bug Fixes * don't init if provider doesn't have an init method ([open-feature#49](open-feature#49)) ([d2479ed](open-feature@d2479ed)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: Firdaus Al Ghifari <firdaus.alghifari@gmail.com> Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com> Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: Firdaus Al Ghifari <firdaus.alghifari@gmail.com> Signed-off-by: wadii <wadii.zaim@flagsmith.com>
🤖 I have created a release *beep* *boop* --- ## [0.0.2](open-feature/ruby-sdk-contrib@openfeature-flipt-provider-v0.0.1...openfeature-flipt-provider/v0.0.2) (2025-04-15) ### 🐛 Bug Fixes * Update Gemfile.lock on flipt provider ([open-feature#55](open-feature#55)) ([fd79d7b](open-feature@fd79d7b)) ### ✨ New Features * Add Flipt provider gem with basic implementation ([open-feature#51](open-feature#51)) ([8e0ae7c](open-feature@8e0ae7c)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Signed-off-by: Firdaus Al Ghifari <firdaus.alghifari@gmail.com> Co-authored-by: Firdaus Al Ghifari <34407426+falghi@users.noreply.github.com> Signed-off-by: wadii <wadii.zaim@flagsmith.com>
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->
## This PR
By default faraday uses net::http which on each request opens new socket
and does all the handshaking every time. Persistent adapter keeps
connection alive for defined amount of time (30s).
fix for `def initialize(options: {})` since it was incorrect on default
behaviour, it is expected to get Options object not hash.
### Follow-up Tasks
With later PR's I could make this configurable.
---------
Signed-off-by: Augustinas Sueris <augustinas.sueris@vinted.com>
Signed-off-by: Augustinas <augustinas.sueris@gmail.com>
Co-authored-by: Max VelDink <maxveldink@gmail.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
…-feature#60) 🤖 I have created a release *beep* *boop* --- ## [0.1.4](open-feature/ruby-sdk-contrib@openfeature-go-feature-flag-provider/v0.1.3...openfeature-go-feature-flag-provider/v0.1.4) (2025-07-30) ### ✨ New Features * connection persistance ([open-feature#59](open-feature#59)) ([3a01a5e](open-feature@3a01a5e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Signed-off-by: wadii <wadii.zaim@flagsmith.com>
## This PR This PR adds [Faraday instrumentation](https://github.com/lostisland/faraday/blob/main/docs/middleware/included/instrumentation.md) configuration. Example: ```ruby OpenFeature::GoFeatureFlag::Options.new( endpoint: "http://localhost:1031", instrumentation: { name: 'custom_name', instrumenter: MyInstrumenter } ) ``` Signed-off-by: avelicka <arunas.velicka@vinted.com> Signed-off-by: wadii <wadii.zaim@flagsmith.com>
…-feature#63) 🤖 I have created a release *beep* *boop* --- ## [0.1.5](open-feature/ruby-sdk-contrib@openfeature-go-feature-flag-provider/v0.1.4...openfeature-go-feature-flag-provider/v0.1.5) (2025-11-04) ### ✨ New Features * **goff:** add Faraday instrumentation ([open-feature#62](open-feature#62)) ([6ae223f](open-feature@6ae223f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Signed-off-by: wadii <wadii.zaim@flagsmith.com>
…reason is DISABLED (open-feature#64) ## This PR Currently, when go-feature-flags has a disabled flag it returns a response in the following format: `{"key":"my_flag","value":"thisisadefaultvaluethatItest1233%%","reason":"DISABLED","variant":"SdkDefault"}`. This `thisisadefault...` comes directly from go-feature-flags. The client package returns this, but sdkDefault makes more sense. https://github.com/thomaspoignant/go-feature-flag/blob/de4d46dd7d513da32a9c67f0c38ae577d51ff6bd/cmd/relayproxy/ofrep/evaluate.go#L95C19-L95C51 Signed-off-by: Dorian de Koning <dorian.koning@vinted.com> Signed-off-by: wadii <wadii.zaim@flagsmith.com>
…-feature#65) 🤖 I have created a release *beep* *boop* --- ## [0.1.6](open-feature/ruby-sdk-contrib@openfeature-go-feature-flag-provider/v0.1.5...openfeature-go-feature-flag-provider/v0.1.6) (2025-11-11) ### 🐛 Bug Fixes * **openfeature-go-feature-flag-provider:** fallback to sdkDefault if reason is DISABLED ([open-feature#64](open-feature#64)) ([6cb08c0](open-feature@6cb08c0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Signed-off-by: OpenFeature Bot <109696520+openfeaturebot@users.noreply.github.com> Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com> Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
94cde03 to
a8e23cf
Compare
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
|
@toddbaert, perfect. Thanks for the fast review, I appreciate! All good with the component-owners and rebase. I struggled a bit with DCO but should be good |
alxckn
left a comment
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 👋 , came across this PR and while it's already been accepted, thought I'd chip in with some potential improvements that stood out to me
providers/openfeature-flagsmith-provider/lib/openfeature/flagsmith/provider.rb
Outdated
Show resolved
Hide resolved
providers/openfeature-flagsmith-provider/lib/openfeature/flagsmith/provider.rb
Outdated
Show resolved
Hide resolved
providers/openfeature-flagsmith-provider/lib/openfeature/flagsmith/provider.rb
Show resolved
Hide resolved
More than happy for more reviewers - like I said, I'm not a Ruby dev! 🙏 |
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
providers/openfeature-flagsmith-provider/lib/openfeature/flagsmith/provider.rb
Show resolved
Hide resolved
providers/openfeature-flagsmith-provider/lib/openfeature/flagsmith/provider.rb
Outdated
Show resolved
Hide resolved
providers/openfeature-flagsmith-provider/lib/openfeature/flagsmith/provider.rb
Outdated
Show resolved
Hide resolved
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
|
Addressed @alxckn. By the way, kudos for your help, I appreciate 👍 |
alxckn
left a comment
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
|
@toddbaert, ready to merge :) Thanks for the help |
@Zaimwa9 recently added open-feature/ruby-sdk-contrib#68 and open-feature/rust-sdk-contrib#82 flagsmith providers. @Zaimwa9 please 👍 or approve this PR if you are interested in joining the org. Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
This PR
Related Issues
Flagsmith issue
Notes
As per an experiment using IA toolings, the code implementation has been 90% done by Claude. Although I have thoroughly supervised and review every step of the process.
Interesting files to grasp the context:
context.mdFLAGSMITH_PROVIDER_DESIGN.mdReadme.mdAt flagsmith, we decided to leave all the working files for complete transparency.
Follow-up Tasks
How to test
bundle exec rspecor