-
Notifications
You must be signed in to change notification settings - Fork 23
fix(flagd): adjust flagd to updated error codes #285
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
fix(flagd): adjust flagd to updated error codes #285
Conversation
Fixing the inconsistencies regarding error codes for edge cases, as described in #1679 Signed-off-by: Konvalinka <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #285 +/- ##
==========================================
- Coverage 94.21% 94.20% -0.02%
==========================================
Files 20 20
Lines 1020 1035 +15
==========================================
+ Hits 961 975 +14
- Misses 59 60 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]> diff --git c/providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/in_process.py i/providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/in_process.py index 3906a2e..50565c6 100644 --- c/providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/in_process.py +++ i/providers/openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/in_process.py @@ -147,8 +147,8 @@ class InProcessResolver: flag_metadata=metadata, reason=Reason.DEFAULT, ) - if isinstance(variant, bool): - variant = str(variant).lower() + + variant = str(variant).lower() # convert to string to support shorthand if variant not in flag.variants: raise GeneralError( f"Resolved variant {variant} not in variants config."
Signed-off-by: Simon Schrottner <[email protected]>
…wise Signed-off-by: Simon Schrottner <[email protected]> Co-authored-by: Lea Konvalinka <[email protected]>
…wise Signed-off-by: Simon Schrottner <[email protected]> Co-authored-by: Lea Konvalinka <[email protected]> diff --git c/providers/openfeature-provider-flagd/tests/e2e/step/context_steps.py i/providers/openfeature-provider-flagd/tests/e2e/step/context_steps.py index 3d3b502..539e71e 100644 --- c/providers/openfeature-provider-flagd/tests/e2e/step/context_steps.py +++ i/providers/openfeature-provider-flagd/tests/e2e/step/context_steps.py @@ -48,6 +48,18 @@ def update_context_without_value( +@given( + parsers.cfparse( + 'a context containing a key "{key}", with type "{type_info}" and with value ""' + ), +) +def update_context_without_value( + evaluation_context: EvaluationContext, key: str, type_info: str +): + """a context containing a key and value.""" + update_context(evaluation_context, key, type_info, "") + + @when( parsers.cfparse( 'context contains keys {fields:s} with values "{svalue}", "{svalue2}", {ivalue:d}, "{bvalue:bool}"',
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
providers/openfeature-provider-flagd/tests/e2e/inprocess/flags/allFlags.json
Outdated
Show resolved
Hide resolved
gruebel
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.
looks good 🍻
...openfeature-provider-flagd/src/openfeature/contrib/provider/flagd/resolvers/process/flags.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Konvalinka <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
Signed-off-by: Simon Schrottner <[email protected]>
| value, | ||
| variant=variant, | ||
| reason=Reason.ERROR, | ||
| error_code=ErrorCode.FLAG_NOT_FOUND, |
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 compared this to the other methods, and there is a special error message used ""Flag '" + key+ "' has no default variant defined, will use code default"" maybe we should adapt the tests to add the message to be in sync in all of them? wdyt? @leakonvalinka
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.
it definitely wouldn't hurt
however i dont know if the error messages are actually used for anything except logs? if not, i think this doesn't have a high priority then
Fixing the inconsistencies regarding error codes for edge cases, as described in #1679