feat: Add disabled flag support to InMemoryProvider #632
Conversation
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
Signed-off-by: Kyle Julian <[email protected]>
kylejuliandev
changed the title
Summary of ChangesHello @kylejuliandev, 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 to the InMemoryProvider by adding explicit support for disabled feature flags. This allows developers to mark flags as inactive, ensuring they consistently resolve to their default values with a 'Reason.Disabled' status. The changes span core flag definitions, JSON parsing for test configurations, and extensive test coverage to guarantee the reliability of this new functionality. 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
|
Summary of ChangesHello @kylejuliandev, 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 enhances the "InMemoryProvider" by introducing explicit support for disabled feature flags. This allows developers to mark flags as inactive, ensuring they consistently resolve to their default values with a "Reason.Disabled", thereby improving clarity and control over feature rollout. The changes span the core "Flag" definition, its evaluation logic, and are thoroughly validated with updated unit and end-to-end tests, alongside a practical example in the sample application. 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #632 +/- ##
==========================================
+ Coverage 89.99% 90.02% +0.03%
==========================================
Files 77 77
Lines 3198 3208 +10
Branches 368 369 +1
==========================================
+ Hits 2878 2888 +10
Misses 251 251
Partials 69 69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 successfully adds support for a disabled flag to the InMemoryProvider, which is a valuable feature. The changes are well-implemented, including updates to the E2E tests to parse and utilize the new disabled property. I have a couple of suggestions to enhance the design and maintain consistency. One is to ensure the immutability of the Disabled property on a flag, and the other is a minor correction in a test file. Overall, this is a solid contribution.
test/OpenFeature.Tests/Providers/Memory/InMemoryProviderTests.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Code Review
This pull request adds support for a disabled flag to the InMemoryProvider, which is a great feature. The implementation looks solid, and the new functionality is well-tested. I've made a few suggestions to improve the code:
- In
src/OpenFeature/Providers/Memory/Flag.cs, I've suggested making theDisabledproperty immutable after creation to prevent potential race conditions and improve predictability. - In
test/OpenFeature.E2ETests/Utils/FlagDictionaryJsonConverter.cs, I found an obsolete comment that should be removed. - In
test/OpenFeature.Tests/Providers/Memory/InMemoryProviderTests.cs, I've made several suggestions to improve the new tests, such as correcting a comment and adding assertions to make them more robust.
Overall, these are minor points in an otherwise excellent contribution. Once these are addressed, the PR should be ready to merge.
test/OpenFeature.Tests/Providers/Memory/InMemoryProviderTests.cs
Outdated
Show resolved
Hide resolved
Signed-off-by: Kyle Julian <[email protected]>
askpt
requested review from
aepfli,
arttonoyan,
beeme1mr,
benjiro,
chrfwow,
kinyoklion and
toddbaert
| /// <summary> | ||
| /// Indicates if the flag is disabled. When disabled, the flag will resolve to the default value. | ||
| /// </summary> | ||
| bool Disabled { get; } |
There was a problem hiding this comment.
@askpt I think the only doubt I have with this change is that it is technically a breaking change. Anyone who may have extended Flag in their projects will have problems upgrading as it won't automatically implement this
There was a problem hiding this comment.
In fairness this is not a "shocking" breaking change. Also should we publish this interface? 🤔
3 participants
This PR
Adds support for Disabled flags in the InMemoryProvider. Also updates the e2e tests to run the tests for disabled flags.
Related Issues
Notes
Follow-up Tasks
How to test