feat: trigger-webhook-events-on-commiting-change-requests #5464
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…agsmith into feat/backend-clone-segments
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…agsmith into feat/backend-clone-segments
for more information, see https://pre-commit.ci
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Docker builds report
|
github-actions
bot
added
front-end
for more information, see https://pre-commit.ci
github-actions
bot
added
feature
github-actions
bot
added
feature
github-actions
bot
added
feature
| self._publish_feature_states() | ||
| self._publish_environment_feature_versions(committed_by) | ||
| self._publish_change_sets(committed_by) | ||
| self._publish_segments() | ||
| self._notify_committed_changes_webhooks(feature_states_to_notify) |
There was a problem hiding this comment.
Similarly to what I'm working on right now in the Sentry integration, I'm concerned about what happens when the change request commits a flag change that will only go live in the future.
The approach I'm taking involves binding events to the "went live" audit log, which was recently fixed to match the actual flag change effect.
Can you please investigate if that's going to be necessary in this context as well?
There was a problem hiding this comment.
The signal feature_state_change_went_live introduced in this commit might help if the above proves necessary.
There was a problem hiding this comment.
Good question.
First: It's triggering a webhook for creating a scheduled change requests => not good, will be changed.
It could make sense, but I could use a bit more of context. As far as I can see, the feature_state_change_went_live is tightly coupled with AuditLog, that is a different type of webhook event and only sends organisation webhooks.
For example trigger_feature_state_change_webhooks is in charge of formatting payloads and sending to both env/org webhooks with WebhookEventType.FLAG_UPDATED. My fear is that by coupling AuditLog and CR Publishing here we re-execute some paths.
I might be overlooking what you had in mind so i'd love some details
There was a problem hiding this comment.
I think it depends on what is the primary event we're dealing with, i.e. to what sending this webhook should trigger to:
- The commit of the change request, or
- The change in the associated feature flag.
If the answer points to (2), then the new feature_state_change_went_live signal I mentioned above might be useful. Yes it creates coupling with audit logs, which I also tried avoiding at first, before I learned that auditing is actually entrypoint to many integrations. cc @khvn26
…ment-flag-update-webhook-on-change-request-commit
github-actions
bot
added
feature
Thanks for submitting a PR! Please check the boxes below:
docs/if required so people know about the feature!Changes
Solves #2064
Webhook events for FLAG_UPDATED were not triggered when committing a Change Request. This PR resolves that issue.
There are two relevant cases:
Feature versioning v2:
Webhooks were intentionally skipped when environment_feature_version_id is present (cf in
test_webhooks_are_not_called_for_feature_state_with_environment_feature_version)Uncommitted Change Requests (this PR's focus):
When creating a Change Request, associated FeatureState objects are created immediately. At this point,
post_savetriggers (now suppressed) were sending a half empty event while the state was not live. However, when the Change Request is committed, the associated feature states are bulk updated—bypassing signals—and no webhook is triggered.I intentionally avoided relying on the post_save signal during commit because Change Request FeatureStates are new records without version when initially created.
Their history reflect the draft state, meaning enabled matches the future committed values. This leads to new_state == old_state in the webhook payload. So resolving FeatureState based on the previous live version ensure that we pass in the correct diff
Fix:
On commit, we now fetch the latest live FeatureState (from DB, not history) and explicitly call trigger_feature_state_change_webhooks() with both the new and previous live state
How did you test this code?