feat: add-flagsmith-provider-crate #82
Conversation
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
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 integrates Flagsmith as a new feature flag provider for the OpenFeature Rust SDK. It allows Rust applications to dynamically evaluate feature flags from Flagsmith, supporting various data types and evaluation contexts. The implementation includes options for local evaluation, robust error handling, and clear documentation, making it a comprehensive solution for feature management within the OpenFeature ecosystem. 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
|
Zaimwa9
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This PR introduces a new Flagsmith provider for the OpenFeature Rust SDK. The implementation is comprehensive, covering all required evaluation types, context mapping, and error handling. The use of a FlagsmithClient trait for mockability and the extensive test suite are excellent.
I've identified a few areas for improvement:
- Error Handling: The current mechanism for detecting a "Flag Not Found" error relies on string matching, which is brittle.
- Code Duplication: There's significant code duplication across the
resolve_*methods that could be refactored. - Context Conversion:
Structvalues in the evaluation context are silently dropped during conversion to Flagsmith traits. - Documentation: Some code examples in the README are not compilable.
- Project Files: The
context.mdfile seems to be an artifact of the development process and might not be intended for inclusion in the repository. - Configuration: The crate's Rust edition is
2021, while the project seems to be moving towards2024.
Overall, this is a great contribution. Addressing these points will improve the provider's robustness and maintainability.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Zaimwa9 <wadii.zaim@gmail.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
…-sdk-contrib into feat/add-flagsmith-provider
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Zaimwa9 <wadii.zaim@gmail.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
…-sdk-contrib into feat/add-flagsmith-provider
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>
|
@gemini-code-assist. Are you able to provide a new review ? |
There was a problem hiding this comment.
Code Review
This pull request introduces a new open-feature-flagsmith provider crate, which integrates the Flagsmith SDK with OpenFeature for Rust. The implementation is comprehensive, covering all OpenFeature flag types, environment and identity-based evaluation, local evaluation mode, and extensive testing. The code is well-structured, documented, and makes good use of Rust's features, including a trait for mocking the Flagsmith client for tests.
My review focuses on improving the robustness and maintainability of the library. I've suggested a few changes:
- Specifying explicit
tokiofeatures inCargo.tomlinstead of usingfullto reduce the dependency footprint for consumers. - Correcting a markdown formatting issue in the
README.mdto improve documentation readability. - Enhancing a code example in the
README.mdto make it self-contained and easier for users to understand. - Adding a missing type check in the
resolve_struct_valuefunction to improve robustness and align it with the other resolver functions.
Overall, this is a great contribution and the new provider is a valuable addition to the OpenFeature ecosystem.
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Zaimwa9 <wadii.zaim@gmail.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Signed-off-by: Zaimwa9 <wadii.zaim@gmail.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
|
@gemini-code-assist, new review please |
There was a problem hiding this comment.
Code Review
This pull request introduces a new crate for the Flagsmith OpenFeature provider. The implementation is of high quality, demonstrating a strong understanding of both OpenFeature and asynchronous Rust principles, especially with the correct use of spawn_blocking for synchronous SDK calls. The code is well-structured, and the inclusion of a mockable client trait and comprehensive tests (unit, integration, and error-specific) is excellent. I've added a few review comments with suggestions to enhance consistency in type handling, remove a piece of unused code, and improve the handling of null values in JSON structures. Overall, this is a solid contribution.
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
|
Hey @Zaimwa9, I've pinged Todd to see if he can help review. We'll only be able to review from an OpenFeature perspective as Rust is not an area of expertise for either of us. |
| /// ``` | ||
| #[instrument(skip(environment_key, options))] | ||
| pub async fn new( | ||
| environment_key: String, |
There was a problem hiding this comment.
We might benefit from use of Secret<String> here, so this can't really be logged.
toddbaert
left a comment
toddbaert
left a comment
There was a problem hiding this comment.
Approving from a provider/spec perspective, with one code recommendation.
Deps look good, docs look good.
Please also merge/rebase main and add a component owners entry as in Ruby: https://github.com/open-feature/rust-sdk-contrib/blob/main/.github/component_owners.yml
|
Thanks @toddbaert, will do. I'll be waiting for an internal review by next week before moving forward |
…add-flagsmith-provider
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
@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>
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
|
@toddbaert , Gagan is part of flagsmith team with knowledge in Rust. I believe we are ready to move forward. |
Signed-off-by: wadii <wadii.zaim@flagsmith.com>
6 participants
This PR
This PR introduces the flagsmith rust provider
Related Issues
Flagsmith related 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.mdREADME.mdAt flagsmith, we decided to leave all the working files for complete transparency.
Follow-up Tasks
How to test
cargo test -p open-feature-flagsmith