-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Record webhook events #11356
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
Record webhook events #11356
Conversation
/werft run 👍 started the job as gitpod-build-at-webhook-events.7 |
@geropl, please have a look on the DB table. |
components/gitpod-db/src/typeorm/migration/1657702361007-AddWebhookEvent.ts
Outdated
Show resolved
Hide resolved
const config = await this.prebuildManager.fetchConfig({ span }, user, context); | ||
if (!this.prebuildManager.shouldPrebuild(config)) { | ||
console.log("Bitbucket push event: No config. No prebuild."); | ||
await this.webhookEvents.updateEvent(event.id, { status: "ignored_unconfigured" }); |
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.
Hm. As the table is about WebhookEvents now this prebuild-specific status does not make sense now: Imagine a webhook triggered both, a preview and a prebuild, what status would it be in?
What about keeping the status
strictly with the WebHook:
"received"
| "dismissed_unauthorized"
| "ignored_incomplete"
| "failed" // generic error during _webhook_ processing
| "processed" // or similar
Still, we need a place to document the decisions. E.g. whether or not a prebuild got triggered is conveyed by the prebuildId
, but still we need to store the "why", ideally in an indexable + a human readable format (think status + message). So far this has been done inside the PrebuiltWorkspace entity, but I really really would like to get away from that. 🤔
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.
prebuild-specific status does not make sense now: Imagine a webhook triggered both, a preview and a prebuild, what status would it be in?
Good question. I think instead of generalizing to a minimal set of commons, I'd rather split the status by concerns. In fact, we need explicit states for the frontend to render. If we coerce different concerns into generic states, I fear it will be difficult to render meaningful states.
What about adding explicit prebuildStatus
? We can tag specific statuses.
Also, is there something to read more on the new preview feature? I find it hard to reason about, as I've no details so far.
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.
The preview feature will. basically start a workspace on those events in a similar context as prebuilds. The main difference is that it keeps running longer (long timeout + lifecycle bound to commit on tip of branch) and exposes a port publicly which is sent back to GH/GL as a check.
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.
I believe the webhook table should not contain any prebuild information at all. In fact I think even the reference should be the other way around. I.e. a prebuild that was triggered based on a webhook should reference it.
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.
I believe the webhook table should not contain any prebuild information at all.
It's certainly a trade off, but I feel that it's worth it. The goal here is to untangle the question "what happened to this prebuild event and why" from the PrebuiltWorkspace
state machine (which feels broken atm).
We could certainly add another table, e.g. WebhookEvent -> PrebuildEvent -> PrebuiltWorkspace, but as we'd join that on primary key of the WebHook event and to avoid the additional complexity here, I think it's ok (for now) to have that inside the WebhookEvent table. 🧘
Could we also already extract the pushing user and match it with our user? We plan to build a metric into the product that shows team activation and will be based on the difference of commits authored in total (the webhook events) vs. which of those were done from Gitpod. So would be great to have a user shape in this table that optionally has a userID if that user exists in d_b_user. |
I think that is the intend, but this PR serves as a first step towards that goal. 👣 👍 |
Also, extracting branch name and commit seem reasonable? 😇 |
9265737
to
f1c3405
Compare
/werft run 👍 started the job as gitpod-build-at-webhook-events.10 |
f1c3405
to
2633e6b
Compare
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.
Code LGTM, tested and works! 🚀
Super exited to have this, thank you @AlexTugarev ! 😍
Description
This PR adds recording of webhook events received for GH/GHE/GL/BB/BBS.
For each
push
event received for any configured repositories, a record will be stored in the DB containing the raw payload necessary to trigger a prebuild at any time. To start with a 🛹, the actual trigger remains untouched here, only the status is updated up the the point of a successful prebuild trigger (or any error case.) The status updates will be already beneficial in the follow up PR to render updates on the dashboard.The proposed DB table is quite simple.
Related Issue(s)
Related #10764
How to test
Push a commit to a repository configured as a project with this preview environment and verify you see webhook events stored in the DB.
Release Notes
Documentation
Werft options: