-
Notifications
You must be signed in to change notification settings - Fork 20
feat(provider): add Unleash provider #301
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
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.
Summary of Changes
Hello @kikihakiem, 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 specifically designed for Unleash, a popular feature management platform. Its primary purpose is to bridge the gap between OpenFeature's standardized API and Unleash's feature flagging capabilities for Python applications. This integration empowers developers to manage feature rollouts and experiments seamlessly, leveraging both OpenFeature's interoperability and Unleash's robust feature management features. The implementation is thorough, covering various flag types, event handling, and tracking, all backed by a comprehensive test suite.
Highlights
- New Provider Added: Introduced a new OpenFeature provider for Unleash, enabling Python applications to integrate with Unleash for feature flagging and experimentation.
- Community Request Fulfilled: This addition directly addresses community requests for OpenFeature support within the Unleash ecosystem.
- Comprehensive Functionality: The provider supports boolean, string, integer, float, and object flag types, along with OpenFeature event handling and tracking capabilities.
- Robust Testing: Includes extensive unit and integration tests, ensuring reliability and correctness of the Unleash provider.
- Development Tooling: New
pyproject.toml
entries and utility scripts are added to streamline development, testing, type checking, and linting workflows.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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 PR adds a new Unleash provider for the OpenFeature Python SDK. The implementation is well-structured and includes a comprehensive set of unit and integration tests, which is great to see. I've left a few comments with suggestions for improvement, mainly around security best practices in helper scripts and removing some potentially unreachable error handling code for better maintainability. Overall, this is a solid contribution.
except requests.exceptions.HTTPError as e: | ||
if e.response and e.response.status_code == 404: | ||
raise FlagNotFoundError(f"Flag not found: {flag_key}") from e | ||
raise GeneralError(f"HTTP error: {e}") from e |
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.
This except requests.exceptions.HTTPError
block is likely unreachable. The UnleashClient.is_enabled()
method evaluates flags based on its in-memory cache and does not perform network requests that would raise an HTTPError
. Network errors during feature fetching are handled by the client's background poller. This exception handling can be removed to simplify the code.
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 this is an accurate comment. Unleash should not be making HTTP calls during is_enabled function. I'd remove this error handling.
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 for the assurance that a network error is unlikely. I'll remove this error handling.
except requests.exceptions.HTTPError as e: | ||
if e.response and e.response.status_code == 404: | ||
raise FlagNotFoundError(f"Flag not found: {flag_key}") from e | ||
raise GeneralError(f"HTTP error: {e}") from e |
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.
Also accurate
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
…eger_details, resolve_float_details and resolve_object_details Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
d5f7e81
to
9a579d6
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.
Thanks for the contribution! I think it's heading in the right direction, but from Unleash design perspective, there are a few things that should be done differently. I believe it's a relatively simple change that will make the provider much better.
except requests.exceptions.HTTPError as e: | ||
if e.response and e.response.status_code == 404: | ||
raise FlagNotFoundError(f"Flag not found: {flag_key}") from e | ||
raise GeneralError(f"HTTP error: {e}") from e |
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 this is an accurate comment. Unleash should not be making HTTP calls during is_enabled function. I'd remove this error handling.
except requests.exceptions.HTTPError as e: | ||
if e.response and e.response.status_code == 404: | ||
raise FlagNotFoundError(f"Flag not found: {flag_key}") from e | ||
raise GeneralError(f"HTTP error: {e}") from e |
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.
Also accurate
self.app_name = app_name | ||
self.api_token = api_token | ||
self.environment = environment | ||
self.client: Optional[UnleashClient] = None |
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 don't think the client should be Optional. It's safe to initialize UnleashClient here, so you'd remove a lot of complexity of handling this being optional.
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 deliberately make UnleashClient
optional and separate UnleashClient
initialization from UnleashProvider
constructor. It allows us to follow the OpenFeature provider lifecycle (NOT_READY
-> READY
-> ERROR
). It also allows unit tests to create providers without immediately connecting to Unleash. Let me know if this is not the case with Unleash or if we don't really need distinction between NOT_READY
and READY
states.
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 the instantiation of UnleashClient does not make it ready. It will switch to ready when it receives the first update from upstream (or loads from bootstrap or backup). Not having it optional would simplify the design and should still be aligned with OF lifecycle. You can find Unleash documentation here: https://github.com/Unleash/unleash-python-sdk/?tab=readme-ov-file#lifecycle-events
Apparently, you can't disable fetching at startup with this SDK, but you shouldn't carry that design choice to this connector. I think you should use the bootstrapping mechanism to have a stable set of features and keep an invalid Unleash API url, which will fail with 404. Increasing the fetch interval could also help, but not with the initial fetch (if the docs are up to date)
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 for your input. I've updated the Unleash lifecycle event handling. Please take a look
if not self._provider.client: | ||
raise GeneralError("Provider not initialized. Call initialize() first.") |
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.
is_enabled and resolve_variant paths should never throw exceptions. This is one of the design principles at Unleash. This would be resolved by removing the optional as in the other comment, and removing the exception handling from this method and is_enabled
Signed-off-by: Kiki L Hakiem <[email protected]>
01b3e09
to
048029b
Compare
Signed-off-by: Kiki L Hakiem <[email protected]>
1931868
to
7e02cb3
Compare
Signed-off-by: Kiki L Hakiem <[email protected]>
Signed-off-by: Kiki L Hakiem <[email protected]>
7e02cb3
to
4133039
Compare
This PR
Adds Unleash Provider for OpenFeature Python SDK
Related Issues
Fixes #300
Fixes #3896 - Addresses community request for OpenFeature support in Unleash ecosystem
Notes
Follow-up Tasks
How to test
uv sync
uv run test -m "not integration"
uv run test -m integration
uv run mypy-check
uv run ruff check