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.
|
|
||
| ### Configure multiple segment overrides | ||
|
|
||
| Set different values per segment — for example, pricing tiers. |
There was a problem hiding this comment.
Can I omit priorities in this case?
There was a problem hiding this comment.
I'd say that we should infer priorities from the ordering in the payload if not provided.
There was a problem hiding this comment.
Which priority numbers could represent ordering in the payload? Are we thinking of something close to INF - n, or a regular 1-based enumeration?
There was a problem hiding this comment.
My initial instinct was to steer away from user input space, but I guess it mostly doesn't matter since we reorder in the UI using drag and drop.
| "segment_overrides": [ | ||
| { | ||
| "segment_id": 456, | ||
| "delete": true |
There was a problem hiding this comment.
Not loving the API here. I'd prefer to have full replace PUT semantics instead — demand the full overrides list, delete omitted elements.
IMO, individual override operations work well with individual APIs, e.g.
DELETE /api/experiments/environments/{environment_key}/update-flag/{feature_id}/segment/{segment_id}PUT /api/experiments/environments/{environment_key}/update-flag/{feature_id}/identity/{identifier}
This option especially makes sense in the CLI V2 context.
There was a problem hiding this comment.
I'd prefer to have full replace PUT semantics instead — demand the full overrides list, delete omitted elements.
This sounds like a can of worms that leads back to needing both option_a and _b. I do agree that I don't like the idea of adding the new "delete": true attribute to the payload, however. I prefer a separate call for this. We had (I think) a /remove-segment-override endpoint previously alongside the /update-flag one, right? I tend to think that Kim's suggestion is a good one though of having DELETE /api/experiments/environments/{environment_key}/update-flag/{feature_id}/segment/{segment_id} .
There was a problem hiding this comment.
IMO, individual override operations work well with individual APIs, e.g.
DELETE /api/experiments/environments/{environment_key}/update-flag/{feature_id}/segment/{segment_id}PUT /api/experiments/environments/{environment_key}/update-flag/{feature_id}/identity/{identifier}
Love it. For some reason, I thought we were constrained to one swiss-army-knifey /update-flag/ endpoint only.
There was a problem hiding this comment.
demand the full overrides list, delete omitted elements.
I think this would break the intended ergonomics of this endpoint — change only these things about this feature, leave out all the rest. I think this intent is only inheritance of Option B's nature, which we chose.
On a second thought, the suggested /update-flag/{feature_id}/ path also breaks ergonomics since it relies on the feature ID, while this endpoint assumes the user may choose to identify features given their name.
So, should we:
- Support either a feature name or ID in the URL?
- Choose to support only
feature_name? - Restore Gagan's separate
POST .../delete-segment-override/but with Option B's style?
There was a problem hiding this comment.
change only these things about this feature, leave out all the rest
My reasoning is: we can't cleanly express this logic for all input lists (overrides, variants); supporting delete: true for all lists doesn't sound like a good idea either; full-replace logic is the only one we can support consistently for both variants and overrides.
On a second thought, the suggested
/update-flag/{feature_id}/path also breaks ergonomics since it relies on the feature ID
{feature_name}, or using either name or id, sounds good to me.
| Set different values per segment — for example, pricing tiers. | ||
|
|
||
| ```bash | ||
| curl -X POST 'https://api.flagsmith.com/api/experiments/environments/{environment_key}/update-flag/' \ |
There was a problem hiding this comment.
nit: Since we're shipping experiments, I think it's best to avoid confusion:
| curl -X POST 'https://api.flagsmith.com/api/experiments/environments/{environment_key}/update-flag/' \ | |
| curl -X POST 'https://api.flagsmith.com/api/experimental/environments/{environment_key}/update-flag/' \ |
But maybe we should bite the bullet already:
| curl -X POST 'https://api.flagsmith.com/api/experiments/environments/{environment_key}/update-flag/' \ | |
| curl -X POST 'https://api.flagsmith.com/api/v2/environments/{environment_key}/update-flag/' \ |
There was a problem hiding this comment.
We don't need v2 though because we're not changing an endpoint - this is a new endpoint. It's under the experiments namespace because it's subject to change, not because it's any sort of breaking change. If we go with v2 and need to change it, we'll have to go to v3 ...
I do agree with the comment on experiments vs experimental though. It might be a PITA to change that now though.
There was a problem hiding this comment.
Arguably, this is the first time this experiments/ namespace is used, and we're updating the URLs anyway, so it should be an easy change.
/api/__future__/environments/{environment_key}/update-flag/{feature_name}
I've borrowed Python's dunder-future name in the pull request title. Not sure if a good disambiguation suggestion for an API path, but it is definitely one.
| The `variants` list in `segment_overrides` can only re-weight existing variants. Variants omitted from it keep their | ||
| current weights for that segment. |
There was a problem hiding this comment.
The
variantslist insegment_overridescan only re-weight existing variants.
👍
Variants omitted from it keep their current weights for that segment.
I am not exactly sure how that's supposed to work. For example, if we send 1 variant with 100% weight, the backend ought to re-weight all other variants to 0?
There was a problem hiding this comment.
Should we instead enforce that the client has to provide all variants?
| Set different values per segment — for example, pricing tiers. | ||
|
|
||
| ```bash | ||
| curl -X POST 'https://api.flagsmith.com/api/experiments/environments/{environment_key}/update-flag/' \ |
There was a problem hiding this comment.
We don't need v2 though because we're not changing an endpoint - this is a new endpoint. It's under the experiments namespace because it's subject to change, not because it's any sort of breaking change. If we go with v2 and need to change it, we'll have to go to v3 ...
I do agree with the comment on experiments vs experimental though. It might be a PITA to change that now though.
|
|
||
| ### Configure multiple segment overrides | ||
|
|
||
| Set different values per segment — for example, pricing tiers. |
There was a problem hiding this comment.
I'd say that we should infer priorities from the ordering in the payload if not provided.
|
|
||
| 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.
| The `variants` list in `segment_overrides` can only re-weight existing variants. Variants omitted from it keep their | ||
| current weights for that segment. |
There was a problem hiding this comment.
Should we instead enforce that the client has to provide all variants?
| "segment_overrides": [ | ||
| { | ||
| "segment_id": 456, | ||
| "delete": true |
There was a problem hiding this comment.
I'd prefer to have full replace PUT semantics instead — demand the full overrides list, delete omitted elements.
This sounds like a can of worms that leads back to needing both option_a and _b. I do agree that I don't like the idea of adding the new "delete": true attribute to the payload, however. I prefer a separate call for this. We had (I think) a /remove-segment-override endpoint previously alongside the /update-flag one, right? I tend to think that Kim's suggestion is a good one though of having DELETE /api/experiments/environments/{environment_key}/update-flag/{feature_id}/segment/{segment_id} .
2b9f91b to
4593ce3
Compare
4593ce3 to
ff05fcf
Compare
c17e1a8 to
468a299
Compare
40914ff to
e111394
Compare
docs/if required so people know about the feature.Changes
Contributes to #7642
Stabilises experimental
update-flagendpoints to one option, still experimental.experimentalnamespace.How did you test this code?
Integration tests included.