chore(preprod): Add github webhook handler for size approve action #106851
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
github-actions
bot
added
the
Scope: Backend
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
NicoHinderling
force-pushed
the
01-22-chore_preprod_add_github_webhook_handler_for_size_approve_action
branch
from
1390e38 to
1b9592a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
NicoHinderling
force-pushed
the
01-22-chore_preprod_add_github_webhook_handler_for_size_approve_action
branch
from
668e3b2 to
f8dc4af
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| Log.APPROVAL_ALREADY_EXISTS, | ||
| extra={**extra, "sibling_artifact_id": sibling.id}, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Duplicate approval check only examines latest approval
Medium Severity
The duplicate approval check queries only the latest_approval (ordered by -id) and compares its GitHub ID with the sender's ID. This prevents duplicates only when the same user's approval is the most recent one. If User A approves, then User B approves, then User A tries to approve again, the check fails because latest_approval is from User B, allowing User A to create a duplicate approval. The test docstring states "Same GitHub user clicking approve twice should not create duplicate," but this logic doesn't fully enforce that invariant.
There was a problem hiding this comment.
that's intentional (cc @trevor-e thoughts)
NicoHinderling
force-pushed
the
01-22-chore_preprod_add_github_webhook_handler_for_size_approve_action
branch
from
d41d978 to
4e90e3c
Compare
NicoHinderling
force-pushed
the
01-22-chore_preprod_add_github_webhook_handler_for_size_approve_action
branch
from
4e90e3c to
3a9ab9f
Compare
| extra = { | ||
| "organization_id": organization.id, | ||
| "external_id": external_id, | ||
| "sender_login": sender.get("login"), |
There was a problem hiding this comment.
I think this will record the user's name, can we use their ID instead to avoid PII?
| metrics.incr(Log.ARTIFACT_NOT_FOUND) | ||
| return | ||
|
|
||
| if artifact.project.organization_id != organization.id: |
There was a problem hiding this comment.
nit: can we include the organization.id as part of the PreprodArtifact query to keep that check atomic? if we don't find one for (artifact_id, org.id) then it should raise PreprodArtifact.DoesNotExist.
| .first() | ||
| ) | ||
|
|
||
| if latest_approval: |
There was a problem hiding this comment.
I don't think this check is needed, there isn't much harm in just creating a new record every time. If you want to keep this we should fix the loop query though, it's possible to get this in a single query with a rank function.
| repo=self.repo, | ||
| ) | ||
|
|
||
| assert PreprodComparisonApproval.objects.count() == 0 |
There was a problem hiding this comment.
Asserts like this might cause leakage between tests and assumes other tests creating PreprodComparisonApproval objects will clean up afterwards. Can we scope the query to the test org?
With this webhook, the status check for size can call this webhook to mark an artifact as approved