feat: Context values for Segments #220
Conversation
khvn26
requested review from
gagantrivedi and
matthewelwell
and removed request for
gagantrivedi
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 58e833a |
| [submodule "tests/engine_tests/engine-test-data"] | ||
| path = tests/engine_tests/engine-test-data | ||
| url = https://github.com/flagsmith/engine-test-data.git | ||
| branch = feat/context-values |
There was a problem hiding this comment.
A discussion point as well as merging Flagsmith/engine-test-data#8 will break CI for all local eval SDKs. It's a good incentive for closing Flagsmith/flagsmith#5676 ASAP, of course, but perhaps we should consider versioning engine-test-data.
There was a problem hiding this comment.
Do you mean hard versions? e.g. v1/ and v2/, etc? I support temporary branch pointing at main — given a reasonable plan — unless there's value in the former.
There was a problem hiding this comment.
I meant using version tags in engine-test-data and referencing them instead of main branch in submodules.
There was a problem hiding this comment.
Interesting. Yes, I think it makes sense to version the test data.
I guess the original thinking behind not versioning it, is that the test data was to be used for more of a regression test, but I don't think that makes sense here, so I'm happy to go with a versioned approach.
There was a problem hiding this comment.
Created https://github.com/Flagsmith/engine-test-data/releases/tag/v1.0.0 for the current SDKs to point to 👍
| context_value: TraitValue, | ||
| ) -> bool: | ||
| return isinstance(trait_value, str) and str(segment_value) not in trait_value | ||
| return isinstance(context_value, str) and str(segment_value) not in context_value |
There was a problem hiding this comment.
Unrelated with this PR but I wonder if we should really "None" not in context_value if segment_value is null.
There was a problem hiding this comment.
Nice catch, let's come up with a test case for this (outside of this PR)?
There was a problem hiding this comment.
I'm writing an issue but is there any use case where segment_value is None other than PERCENTAGE_SPLIT? Do you see how to reproduce this with a real scenario? I think potentially this has never been a problem.
There was a problem hiding this comment.
Yeah, I don't think we can have an IN segment with a null value, realistically.
khvn26
force-pushed
the
feat/context-values
branch
from
ea3f342 to
93c2310
Compare
khvn26
force-pushed
the
feat/context-values
branch
from
93c2310 to
42829fa
Compare
There was a problem hiding this comment.
Looks neat overall, added a couple of comments, and keen to see the resolutions on the outstanding comments from @emyller .
| [submodule "tests/engine_tests/engine-test-data"] | ||
| path = tests/engine_tests/engine-test-data | ||
| url = https://github.com/flagsmith/engine-test-data.git | ||
| branch = feat/context-values |
There was a problem hiding this comment.
Interesting. Yes, I think it makes sense to version the test data.
I guess the original thinking behind not versioning it, is that the test data was to be used for more of a regression test, but I don't think that makes sense here, so I'm happy to go with a versioned approach.
| ) | ||
| for rule in segment.rules | ||
| return bool(rules := segment.rules) and all( | ||
| context_matches_rule(context=context, rule=rule, segment_key=segment.id) |
There was a problem hiding this comment.
This part is particularly interesting (cc @gagantrivedi). This is something that I was chatting to Gagan about regarding the release pipelines work - the reason being is that we can't currently implement a phased rollout using the functionality available in release pipelines since we'd need to use separate segments for each percentage increment. If we allow users to link segments using some kind of other key, then we'd be able to achieve this.
All this is to say, I like the abstraction of segment_key - I don't think there's any changes necessary here right now, I'm just calling it out for Gagan's attention more than anything.
I also don't know if using multiple segments is the right approach for phased rollouts anyway!
Closes #219.