feat(__future__): Experimental update-flag - #8102
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded an experimental future API for reading and updating feature flags. The API supports PATCH and PUT operations for environment defaults, segment overrides, variant allocations, replacement behaviour, permissions, workflow rejection, version publication, audit logging, and structured events. Added serializers, typed schemas, response mappers, integration tests, OpenAPI schemas, and documentation. Shared feature value mappings now support API serialisation. Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟠 High · up to This PR adds new flag-update behavior, but the current implementation can expose internal validation details and persist incorrect flag configuration when duplicate variants or conflicting segment priorities are submitted. These security and correctness risks should be fixed before merging; documentation and error-reporting issues also remain for 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 |
github-actions
Bot
added
api
khvn26
left a 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.
2b9f91b to
4593ce3
Compare
github-actions
Bot
added
feature
4593ce3 to
ff05fcf
Compare
github-actions
Bot
added
feature
c17e1a8 to
468a299
Compare
github-actions
Bot
removed
feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6fbf2c9d-7753-420e-8ab4-93a8a84bfcfb
📒 Files selected for processing (21)
api/api/urls/future.pyapi/app/urls.pyapi/experimentation/services.pyapi/features/feature_states/models.pyapi/features/future/__init__.pyapi/features/future/exceptions.pyapi/features/future/mappers.pyapi/features/future/permissions.pyapi/features/future/serializers.pyapi/features/future/services.pyapi/features/future/types.pyapi/features/future/views.pyapi/tests/integration/conftest.pyapi/tests/integration/environments/identities/test_integration_identities.pyapi/tests/integration/features/future/__init__.pyapi/tests/integration/features/future/test_update_flag_endpoint.pyapi/tests/integration/helpers.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/integrating-with-flagsmith/flagsmith-api-overview/admin-api/updating-flags.mddocs/docs/managing-flags/feature-versioning.mddocs/docs/managing-flags/updating-flags.md
💤 Files with no reviewable changes (1)
- docs/docs/integrating-with-flagsmith/flagsmith-api-overview/admin-api/updating-flags.md
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
api/features/future/mappers.py (1)
14-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExplain the
arg-typeignore.The ignore at Line 18 suppresses Mypy without stating the local type invariant. Add a short explanation, or remove the ignore if the shared mapping already satisfies the response type. (raw.githubusercontent.com)
Based on learnings, API typing workarounds should use precise Mypy ignores with a short explanation so obsolete ignores can be removed.
Source: Learnings
api/features/future/serializers.py (1)
53-65: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject duplicate variant IDs.
given_idsis a set, so duplicate IDs disappear before the membership checks._write_variantsthen processes the same option more than once. The request can pass validation but persist a different allocation from the validated weights. (raw.githubusercontent.com)Check that the number of unique IDs equals the number of variants.
Proposed validation
given_ids = {variant["id"] for variant in variants} + if len(given_ids) != len(variants): + raise serializers.ValidationError("Duplicate variant.")api/features/future/services.py (1)
281-287: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for API-key publication.
This branch passes
author.keytopublished_by_api_key, which is different from theFFAdminUserpath. Add a v2-versioned integration test with a master API key and assert the published version author fields. (raw.githubusercontent.com)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c19b85e-9abf-44da-9f91-025e013ac74a
📒 Files selected for processing (6)
api/features/future/mappers.pyapi/features/future/serializers.pyapi/features/future/services.pyapi/features/future/views.pydocs/docs/deployment-self-hosting/observability/_events-catalogue.mddocs/docs/managing-flags/updating-flags.md
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8102 +/- ##
==========================================
+ Coverage 98.76% 98.78% +0.01%
==========================================
Files 1594 1604 +10
Lines 63657 64335 +678
==========================================
+ Hits 62873 63551 +678
Misses 784 784 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
khvn26
left a comment
khvn26
left a comment
There was a problem hiding this comment.
Some comments to resolve before I approve.
6 participants
docs/if required so people know about the feature.Changes
Contributes to #7642
Closes #8088
Stabilises experimental
update-flagendpoints to one option, still experimental. It supports the verbsPATCHandPUT.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.