-
Notifications
You must be signed in to change notification settings - Fork 458
feat: initial PoC for new endpoint to update a flag, regardless of versioning #6221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (92.66%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #6221 +/- ##
==========================================
- Coverage 98.01% 97.99% -0.02%
==========================================
Files 1278 1280 +2
Lines 45138 45285 +147
==========================================
+ Hits 44240 44378 +138
- Misses 898 907 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| default="string", | ||
| ) | ||
|
|
||
| segment_id = serializers.IntegerField(required=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to provide priority here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed we @gagantrivedi, we plan to focus on the environment default case for now, and ignore for segments for now. The logic behind this is it creates a much more streamlined endpoint here for users that don't care about segments.
We could then either:
- Update this endpoint to include segment information if we get feedback that it should
- Create another similar endpoint which handles this for a specific segment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On further discussions, we have gone back to accommodating segment overrides in this endpoint.
We should make priority optional (if the priority is missing, it just gets added to the end of the list). We see that there are 2 main use cases here:
- Send
"priority": 1to add it to the top of the list - Send
"priority": null(or don't send) and the segment is added to the end of the list.
The endpoint should handle creating or updating a segment override, and should return in the response payload whether the change was an update or not (the status code should still be 200).
| name="edge-identity-overrides", | ||
| ), | ||
| path( | ||
| "<int:environment_id>/features/<str:feature_name>/update-flag/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, as discussed with @gagantrivedi, let's remove the feature name from the URL since there is nothing stopping it containing HTTP reserved characters. The endpoint should therefore be:
POST /environments/:key/update-flag
{
"feature_name": "foo",
...
}
|
Closing this PR in favour of creating a more complete version as part of #6233 |
Changes
Contributes to #6029
Adds a new endpoint to update a flag in a given environment, regardless of whether versioning is used or not.
The reasoning behind creating a new endpoint is:
The new endpoint will look something like this:
Other thoughts / outstanding questions:
a. How does the current create-segment-override endpoint currently behave in a versioned environment?
a. We need to consider the following scenarios here:
i. Flag is in a release pipeline - this should definitely error
ii. Flag has a scheduled change - should this error?
iii. Change requests are enabled for the environment.
Based on question (2), we could instead have the endpoint look something like:
Or even, taking (2) into account:
Answered questions
Looking into this, the existing endpoint relies on sending a PUT to the endpoint
/environments/:key/featurestates/:idwhich relies on the ID of the feature state itself. Since we'd be creating a new feature state as part of the logic for a v2 versioned environment (and hence changing the ID), this would either be really bad UX, or just wouldn't work.We agreed the URL shouldn't include the feature at all, and the name should be included in the payload. The URL will just be
/environments/:key|id/update-flagHow did you test this code?
Added a basic unit test, but more tests will need to be added when this becomes a real change that we want to make.