feat: Make flagd defaultVariant optional#347
feat: Make flagd defaultVariant optional#347marcozabel wants to merge 1 commit intoopen-feature:mainfrom
Conversation
Summary of ChangesHello @marcozabel, 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 introduces a significant enhancement by making the Highlights
Changelog
Activity
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
|
![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 pull request introduces a valuable feature by making the defaultVariant in flagd optional, and the related code changes in in_process.py and ofrep/__init__.py correctly implement the fallback to the SDK-provided default value. However, I've identified a few issues that need attention. There's a critical typo in a gRPC service name in grpc.py which could disable retry policies. Additionally, config.py contains a confusing double assignment to self.port that should be refactored for clarity, and a significant reduction in DEFAULT_RETRY_BACKOFF_MAX which may be a typo.
providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/grpc.py
Outdated
Show resolved
Hide resolved
providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/config.py
Outdated
Show resolved
Hide resolved
| DEFAULT_RESOLVER_TYPE = ResolverType.RPC | ||
| DEFAULT_RETRY_BACKOFF = 1000 | ||
| DEFAULT_RETRY_BACKOFF_MAX = 120000 | ||
| DEFAULT_RETRY_BACKOFF_MAX = 12000 |
There was a problem hiding this comment.
The value of DEFAULT_RETRY_BACKOFF_MAX has been reduced from 120000ms (2 minutes) to 12000ms (12 seconds). This is a significant change. Could you please confirm if this is intentional or a typo?
|
This is blocked by open-feature/flagd#1862. We will have to release these in relatively quick succession to keep behavior in-line. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #347 +/- ##
==========================================
- Coverage 96.55% 95.68% -0.87%
==========================================
Files 5 30 +25
Lines 174 1459 +1285
==========================================
+ Hits 168 1396 +1228
- Misses 6 63 +57 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
toddbaert
force-pushed
the
feat/make-default-variant-optional
branch
2 times, most recently
from
03a678b to
db70f3f
Compare
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
toddbaert
force-pushed
the
feat/make-default-variant-optional
branch
from
19d4901 to
100c989
Compare
| elif t == TestProviderType.FORBIDDEN: | ||
| launchpad = "forbidden" | ||
| options["port"] = container.get_port(9212) | ||
| options["fatal_status_codes"] = ["FORBIDDEN"] |
There was a problem hiding this comment.
Q: why are we adding something to the fatal status codes list here? Also there's no launchpad config for this, so we shouldn't set it to something different than "default"
(I know this is about a different feature so non-blocking question, as this test is disabled anyways)
leakonvalinka
left a comment
leakonvalinka
left a comment
There was a problem hiding this comment.
I wrote a comment with a question about the fatal status code, but since this PR isn't about that and the corresponding test is disabled anyways, this is non-blocking of course.
5 participants
This PR
Related Issues
Fixes #1234523
Notes
Follow-up Tasks
How to test
Fixes: #348