feat(__future__): Experimental update-flag - #8102
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
khvn26
left a comment
There was a problem hiding this comment.
Looks good overall. My main gripe is combining full-replace and delete: true semantics for lists of different entities in a single API.
|
|
||
| Set up features with weighted variants and customise weights per segment. | ||
|
|
||
| The `variants` list in `environment_default` defines the available variants for the feature, and their default weights |
There was a problem hiding this comment.
I'm not sure I agree that modifying the variants themselves should be part of this endpoint at all. variants (or MultivariateFeatureOptions) are project level concepts. This endpoint should be concerned only with environment level entities.
IMO we should raise an error if a variant key is provided that doesn't already exist.
There was a problem hiding this comment.
I actually thought this was implied by
The variants list in segment_overrides can only re-weight existing variants.
There was a problem hiding this comment.
That's not how I understood it - I understood that there was a difference in behaviour between the environment_default section of the payload, and the segment_overrides on this matter. TBC by @emyller .
There was a problem hiding this comment.
My honest understanding of adding MVs support to update-flag was that it could be used to configure a multivariate experiment. If it's not, I think I need to learn more about the use case.
There was a problem hiding this comment.
Leaning towards knowledge I'm yet to learn: e111394
There was a problem hiding this comment.
Current experiments UI does not allow to add, delete or modify variants — unless I'm missing something, it consumes existing ones.
2b9f91b to
4593ce3
Compare
4593ce3 to
ff05fcf
Compare
c17e1a8 to
468a299
Compare
3749d64 to
ae8274e
Compare
ae8274e to
81077d9
Compare
|
This should be ready for a new review: since 81077d9, I've updated the proposed documentation to make it consistent to our [offline] discussion. Will update tests and move right on to implementing it once this looks good. I've opted to let go of a separate |
|
I guess my only question at this point is whether we stick with |
I decided not to support feature names in this endpoint, for now, because we know no consumer that would benefit from it yet. The proposed docs assumes this discussion never existed, and includes |
docs/if required so people know about the feature.Changes
Contributes to #7642
Stabilises experimental
update-flagendpoints to one option, still experimental.experimentalnamespace.Updated documentation: https://docs-git-feat-update-flag-option-c-flagsmith.vercel.app/managing-flags/updating-flags
How did you test this code?
Integration tests included.