Skip to content
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

Handle and persist checkrun webhook events. #1168

Merged
merged 2 commits into from
May 21, 2021

Conversation

ajshepley
Copy link
Contributor

@ajshepley ajshepley commented May 4, 2021

Ref: #1167

When we receive a status webhook event, we use the webhook data to persist the statuses. This helps with timeliness.

However for checkruns, we wait for a CheckSuite event, which does not contain checkrun information, and we enqueue a refresh which will then retrieve checkrun information.

It turns out that we are also receiving CheckRun webhooks, which do provide enough data to create a checkrun record.

So this PR adds a handler for those events, and attempts to persist them using the existing logic.

Paired with the target PR #1167, this should make Shipit-engine more robust against stale and incorrect data from GitHub's end, by ensuring that we commit. It should also make it a bit faster in responding to checkrun changes, though in practice, the refresh job is quite fast already (fast enough to cause the issues mentioned in #1167).

Alternative Option

Instead of persisting the checkrun, we could also enqueue a refresh job just as CheckSuiteEvent does. But since the target PR adds guards for webhook ordering and staleness, I see no large drawbacks in just persisting the data we've received in this webhook.

In theory, every checkrun event would have checksuite events nearby, but in practice this does not appear to be the case, so persisting the "more correct" data we seem to receive from webhooks, and deferring to it based on timestamps, seems like a good improvement to make.

@ajshepley ajshepley requested a review from a team May 4, 2021 23:54
@ajshepley ajshepley closed this May 4, 2021
@ajshepley ajshepley reopened this May 4, 2021
@JackTLi
Copy link
Contributor

JackTLi commented May 5, 2021

Just dropping by for fun, this looks cool!

@ajshepley
Copy link
Contributor Author

Thanks Jack!


Very strangely, the head commit of this PR has a failing status, but the PR itself is somehow green.

It seems like re-running the GitHub Actions check only applies the statuses to the PR, rather than to the head commit of the PR.

Interesting quirk...

@ajshepley ajshepley force-pushed the ajs/guard_against_stale_checkrun_webhooks branch from cdfdb85 to ae6bf54 Compare May 7, 2021 17:16
@ajshepley ajshepley force-pushed the ajs/handle_persist_checkrun_webhooks branch from 391b951 to e8b5f82 Compare May 7, 2021 17:17
@ajshepley ajshepley force-pushed the ajs/guard_against_stale_checkrun_webhooks branch from ae6bf54 to a8e3d85 Compare May 7, 2021 18:36
@ajshepley ajshepley force-pushed the ajs/handle_persist_checkrun_webhooks branch from e8b5f82 to 90e8bd6 Compare May 7, 2021 18:36
@ajshepley ajshepley force-pushed the ajs/guard_against_stale_checkrun_webhooks branch from a8e3d85 to ff200e6 Compare May 7, 2021 20:08
@ajshepley ajshepley force-pushed the ajs/handle_persist_checkrun_webhooks branch from 90e8bd6 to 01e77ad Compare May 7, 2021 20:14
@ajshepley ajshepley force-pushed the ajs/handle_persist_checkrun_webhooks branch from 01e77ad to 6b86c5d Compare May 17, 2021 20:10
@ajshepley ajshepley force-pushed the ajs/guard_against_stale_checkrun_webhooks branch from ff200e6 to cd85e87 Compare May 17, 2021 20:10
@ajshepley ajshepley force-pushed the ajs/handle_persist_checkrun_webhooks branch from 6b86c5d to 1b6383f Compare May 17, 2021 20:11
@ajshepley ajshepley force-pushed the ajs/guard_against_stale_checkrun_webhooks branch from 3d4e3fb to 22b0ed7 Compare May 17, 2021 21:13
@ajshepley ajshepley force-pushed the ajs/handle_persist_checkrun_webhooks branch from 1b6383f to fa848c7 Compare May 17, 2021 21:16
@ajshepley ajshepley force-pushed the ajs/guard_against_stale_checkrun_webhooks branch from 22b0ed7 to ff43003 Compare May 20, 2021 21:57
@ajshepley ajshepley force-pushed the ajs/handle_persist_checkrun_webhooks branch from fa848c7 to e9c2501 Compare May 20, 2021 22:10
Base automatically changed from ajs/guard_against_stale_checkrun_webhooks to master May 21, 2021 15:08
@ajshepley ajshepley force-pushed the ajs/handle_persist_checkrun_webhooks branch from e9c2501 to 1a3024e Compare May 21, 2021 15:46
@ajshepley ajshepley force-pushed the ajs/handle_persist_checkrun_webhooks branch from 1a3024e to da80e3a Compare May 21, 2021 20:52
@ajshepley ajshepley merged commit 1c41a46 into master May 21, 2021
@ajshepley ajshepley deleted the ajs/handle_persist_checkrun_webhooks branch May 21, 2021 20:57
@shopify-shipit shopify-shipit bot temporarily deployed to rubygems May 21, 2021 21:02 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants