feat: Migrate to the new update-flag contract, and flag update --weight - #92
feat: Migrate to the new update-flag contract, and flag update --weight#92khvn26 wants to merge 13 commits into
flag update --weight#92Conversation
…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%.
|
Warning Review limit reached
Next review available in: 13 minutes Limit details: You’ve used all 1 included review currently available under your plan. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe update command now uses typed PATCH and DELETE API operations with partial state changes. It validates 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: 🔵 Low · up to The PR migrates flag updates to a new partial-write endpoint, but the command test double still contains an unreachable PUT path and does not fully match the endpoint it models, so tests may miss regressions in update behavior. This is a bounded merge-readiness risk that should have explicit owner follow-up. 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).
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
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.
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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmd/cmd_test.go (1)
671-700: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unreachable PUT replacement path.
The handler registers only PATCH, so
r.Method == http.MethodPutis always false. Register PATCH directly and remove thereplaceparameters and PUT-only branches fromapplyFlagUpdateandwriteScope.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b2314422-091c-4e68-bccb-a845b575ef99
📒 Files selected for processing (5)
internal/api/client.gointernal/api/client_test.gointernal/cmd/cmd_test.gointernal/cmd/flag_update.gointernal/cmd/sentinel_test.go
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review.
Every flag mutation goes through endpoints that arrive in Flagsmith
2.263.0, so against an older instance they all failed with
Error: PATCH …/api/__future__/… returned 404 Not Found
Think this shouldn't happen? Tell us: …/issues/new
which sends a self-hosted user to our issue tracker for a supported
configuration. They now get:
Error: changing flags needs Flagsmith 2.263.0 or newer: this instance runs 2.259.0
Upgrade the instance, or point --api-url at one new enough. Reading
flags works on any version.
The version comes from `/version`, read only on this failure path, and
left out of the message when it can't be read.
Detection is the 404 carrying no detail — nothing rendered one, so the
route isn't there. The endpoints answer 404 themselves for an environment
or feature the caller can't see, and those must keep reading as not-found:
they are the common case on SaaS, where "upgrade" would be nonsense. The
same distinction is what tells a missing route from a missing override
when deleting.
Not a version pre-flight: staging serves these endpoints while reporting
2.262.0, being built from main between releases, and a self-hosted
instance may run any tag at all.
Two conflicts, both in the hint machinery #86 reworked: - `errors.go`: main replaced several hint constants with functions that name the host-scoped variable, and added `hintAPIURL`. Took main's block, dropped the `hintEnvironmentKey` constant it turned into a function, and kept `hintUpgradeInstance` beside them. - `sentinel_test.go`: both sides added sentinels. Kept all of them — main's `auth.ErrNoDiscovery` and this branch's two.
The merge brought in `api.ServerVersion`, added by #86 for the same purpose, so the version lookup added here was a second one in the same package. Uses the shared function instead, which also stops going through the 301 that `/version` (no trailing slash) answers with. Adopts its rule about the tag, too: an instance reporting "latest" or a commit sha now has its tag left out of the message rather than quoted back as though it were a version. Nothing here rests on the tag — the 404 already established the endpoint is absent — so it only decides whether the version can be named, where `login` has to stay silent entirely because it is inferring age. Dropped the `package_versions` fallback with it: it would be a change to what `login` gates on, which wants deciding on its own terms.
2 participants
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 and #8307 are both deployed to staging, not yet to SaaS production.
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 too, once #8307 reached staging: the surviving overrides keep their weights and priorities untouched, with nothing restated. An earlier local run turned up the segment-naming bug fixed in 8d82b56.