feat: Migrate to the new update-flag contract, and flag update --weight - #92
Draft
khvn26 wants to merge 9 commits into
Draft
feat: Migrate to the new update-flag contract, and flag update --weight#92khvn26 wants to merge 9 commits into
flag update --weight#92khvn26 wants to merge 9 commits into
Conversation
…ight` The experimental update-flag endpoints are being consolidated into one (Flagsmith/flagsmith#8102), and the CLI's flag mutations move to it: PATCH|PUT /api/__future__/environments/{key}/features/{feature_id}/ The feature moves from the body to the path, so mutations resolve it to an id; segments are named by `{"segment": {"id": n}}`; a refused write is a 400 with a `detail` rather than a 403; and the response carries the flag's whole state in the environment, so the post-update render reads what the server actually wrote. PATCH writes only the properties it carries. That removes the read-then- echo the old endpoint forced on every call: `--enable` no longer restates the value, a segment edit no longer restates the environment default, and `flag reorder` sends priorities alone — so a move can no longer disturb a state, and it needs one request fewer. Deleting an override loses its dedicated endpoint: a PUT replaces the set and whatever it leaves out is gone. So `flag delete --segment` reads the survivors and restates them in full, since a partial echo would reset the state it omits. That makes it a read-modify-write, and a concurrent change to another override is lost — the tradeoff the contract imposes. In exchange, a segment with no override is now caught before the write. `--weight <key|id>=<percentage>` re-weights a multivariate flag, per environment or per segment, closing #46. Weights are per scope, so they are read from the scope's feature state rather than from the variants' project-level defaults, and merged: a partial `--weight` re-weights what it names and keeps the rest, which the endpoint requires anyway since it rejects a variant list that isn't complete. Everything the endpoint would take as a new variant, or reject, is caught first — an unknown key or id, a total over 100, and `control`, which is not a variant but the share the weights leave unallocated. The flag detail views grow the Variants block the RFC describes, showing the weights in force for that scope; it costs one extra read, and only for a feature that has variants. The endpoint is not implemented server-side yet — #8102 is tests, types and docs — so this is verified against the contract those tests describe, not against a live API.
The RFC marked weights with a % in the flag views but not the feature ones, so the same table read two ways. Human output now marks them all; JSON keeps numeric weights, and a weight echoed back as the user typed it (`--weight hero=40`) or read inside a sentence that carries its own % is still bare. RFC amended to match: the three `feature` examples in §9 now show 30%/50%.
|
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:
📝 WalkthroughWalkthroughThe update command now uses typed PATCH and PUT API operations with partial state changes and complete segment-override replacement. It supports validated multivariate variant weight updates and preserves returned server state. Reorder and deletion operations use response data and numeric feature paths. Flag and segment detail views now display scoped variant allocations with percentage formatting. Tests cover request semantics, workflow gating, validation errors, override replacement, reordering, and variant allocation behaviour. Estimated code review effort: 5 (Critical) | ~90 minutes Merge Risk: ⚪ Minimal · up to The PR updates flag mutation behavior and adds scoped variant weights with corresponding tests; no actionable merge-blocking risk remains after normal checks and review. 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
changed the title
flag update --weightflag update --weight
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a57d9dae-07d7-4929-bc8c-df81130e0e80
📒 Files selected for processing (9)
README.mdinternal/api/client.gointernal/api/client_test.gointernal/cmd/cmd_test.gointernal/cmd/feature.gointernal/cmd/flag_identity.gointernal/cmd/flag_reorder.gointernal/cmd/flag_update.gointernal/cmd/flags.go
A new override has no feature-segment row yet, so there was no name to read
for the confirmation or the detail view, and both showed a bare id — right
after the user had named the segment themselves:
✓ Set banner_copy weights to hero=100, sub=0 for segment 1 in ...
Segment 1
Resolving a name warms the segment name cache, so read the name back from
there, and keep the bare id for the case that is genuinely nameless: an
override referenced by id that does not exist yet.
Found testing against the update-flag implementation (Flagsmith/flagsmith#8102).
4 tasks
The merged endpoint answers a workflow-gated write with 409 and `"code": "change_requests_enabled"`, where the branch this was built against used 400 and prose. Matching prose would have broken on the next rewording anyway, so read the code, and don't lean on the status alone — 409 will cover any conflict added later. Without this, an environment that simply uses change requests reports as something to report as a bug.
`--weight hero=10,30011=20` names one variant twice when 30011 is hero's id. The duplicate check keyed on the reference as typed, so this passed and the last weight quietly won. Check once the reference has resolved, and name both spellings in the error — the point is that the user probably doesn't realise they are the same variant.
Deleting an override replaces the whole set, and a replacing write that omits `variants` inherits the environment default's weights rather than leaving the override alone. So an override with no allocations of its own could not be restated by omission, as it was — it came back weighted like the environment. Send the full list for any feature with variants, zeros included.
Priorities order the overrides but needn't be contiguous — the API's own examples use 10/20/30 — and the merged endpoint now rejects two overrides sharing one. Two things followed from the old assumption: - a new override joined at `num_segment_overrides`, which collides with an existing override whenever the priorities are sparse. It now joins past the highest, read from the rows already fetched for the segment's name. - `--priority` was range-checked against that count, rejecting a move the server would accept. Only a negative is rejected now; the server owns the rest.
khvn26
changed the title
flag update --weightflag update --weight
khvn26
marked this pull request as ready for review
August 15, 2026 11:33
khvn26
changed the title
flag update --weightflag update --weight
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16a3a368-a407-4e8c-9ba8-a0f6af9e233d
📒 Files selected for processing (5)
internal/api/client.gointernal/api/client_test.gointernal/cmd/cmd_test.gointernal/cmd/flag_update.gointernal/cmd/flags.go
The check that a new override's detail names its segment was a negative assertion on `"Segment 42\n"` — an exact run of four spaces, which tabwriter sets from the widest label in the view. Any change to the padding would have made it pass while testing nothing. Match the row instead, with `\s+` for the padding, so the assertion says where the name has to appear rather than only that the bare id is absent from one particular rendering.
khvn26
marked this pull request as draft
August 15, 2026 13:53
Flagsmith/flagsmith#8307 adds `DELETE …/features/{id}/segment-overrides/{segment_id}/`, so removing one override no longer means replacing the whole set. That takes the read-modify-write out of `flag delete --segment`: the surviving overrides are neither read nor restated, so a concurrent change to one can't be lost, and the endpoint's answer is the whole flag, as it is for the writes. `echoOverrides` and `echoVariants` go with it — restating an override's weights only mattered because a replacing write inherits the environment default's when `variants` is omitted. The override rows are still read, for two reasons a 404 doesn't cover: the prompt names the segment, and a segment with no override should be caught before asking rather than after.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #46. Moves every
flagsmith flagmutation to the consolidated endpoint from Flagsmith/flagsmith#8102, and adds variant weights.PATCH /api/__future__/environments/{key}/features/{feature_id}/— feature in the path, and a response carrying the flag's whole state, so an update renders what the server wrote.--enableleaves the value alone, andflag reordersends priorities only — one request fewer, and a move can no longer disturb a state.flag delete --segmentuses the endpoint's ownDELETEverb (feat(__future__): Delete a flag's segment override flagsmith#8307), so removing one override neither reads nor restates the others.--weight <key|id>=<percentage>, per environment or per segment, merged onto that scope's own weights. Unknown keys, a variant named twice, totals over 100 andcontrolall fail before the write.%. RFC amended to match.Needs the backend: #8102 is on staging, and the
DELETEverb is still in review as #8307.How did you test this code?
Unit tests for the wire contract, and command tests against a fake API modelling the endpoint's semantics — partial writes, per-scope weight merging, and deletion.
Then end to end against staging on a v2-versioned environment: weights per environment and per segment, override creation, priority moves, reorder, and the change-request refusal. Deletion was exercised the same way against #8307's branch locally, confirming the surviving overrides keep their weights and priorities untouched. An earlier local run turned up the segment-naming bug fixed in 8d82b56.