feat: Make defaultVariant and value optional#331
feat: Make defaultVariant and value optional#331marcozabel wants to merge 6 commits 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 refines the flag evaluation process by ensuring that when a 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.
Code Review
This pull request updates Gherkin test cases to reflect a change in behavior for flags without a defaultVariant. The new expectation is to return a DEFAULT reason instead of a FLAG_NOT_FOUND error. The changes in the test scenarios correctly capture this new logic. My review includes suggestions to remove a couple of redundant test cases that were introduced as a result of these changes, which will help improve the clarity and maintainability of the test suite.
|
I think we only want to change the |
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
Signed-off-by: marcozabel <marco.zabel@dynatrace.com>
7e4d20b to
8a919e5
Compare
| And a context containing a key "email", with type "String" and with value "<email>" | ||
| When the flag was evaluated with details | ||
| Then the resolved details value should be "<resolved_value>" | ||
| And the reason should be "<reason>" |
There was a problem hiding this comment.
| And the reason should be "<reason>" | |
| And the reason should be "<reason>" | |
| And the variant should be null |
@marcozabel @leakonvalinka I think one of the new differences in the spec is we will be explicitly not setting the variant in the no-default cases. I'm not 100% sure if what I've suggested above is the best way to do this matcher (it does match an existing step in the suite already) or if we need a new step like "And the variant should not be set" (depends on how easy the unquoted null will be compared to the existing matcher in the language implementations).
There was a problem hiding this comment.
We cannot add this simple step because for one of the scenarios (TARGETING MATCH) the variant is set, so we would need to make it another variable. I'm not sure how the null is handled either, we will have to try it out I guess :D
This PR
Updates the test cases to use the DEFAULT reason if no defaultVariant was set or the targeting rules did not evaluate a match.
Related Issues
open-feature/flagd#1856
Notes
Follow-up Tasks
How to test