Skip to content

fix(github): Don't fail when there are queued check suites#117

Merged
BYK merged 4 commits intomasterfrom
byk/fix/github-check-suites-pending
Aug 11, 2020
Merged

fix(github): Don't fail when there are queued check suites#117
BYK merged 4 commits intomasterfrom
byk/fix/github-check-suites-pending

Conversation

@BYK
Copy link
Member

@BYK BYK commented Jul 17, 2020

When the check suites are queued but the check runs haven't started yet, Craft immediately fails. With this patch, it also checks the Check Suites API to see if there are any pending check suites.

Fixes #111.

When the check suites are queued but the check runs haven't started yet, Craft immediately fails. With this patch, it also checks the Check Suites API to see if there are any pending check suites.

Fixes #111.
@BYK BYK requested review from jan-auer and tonyo July 17, 2020 15:15
Copy link
Member

@jan-auer jan-auer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm a bit curious how the suites are registered so much earlier than the runs. My guess is that #111 can still happen, it's maybe just less likely. Would it make sense add a guard that waits for some time if there are no check suites?

@BYK based on this patch, could we get rid of the Check Runs API and use the status and conclusion of the suites instead?

suite =>
// Need the any cast as octokit lacks this prop in its types
(suite as any).latest_check_runs_count > 0 &&
suite.status === 'queued'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This is done in two places and might qualify for a small helper function given the comment.

* Gets revision checks from GitHub Check runs API
*
* API docs:
* https://developer.github.com/v3/checks/suites/#list-check-suites-for-a-git-reference
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this requires the checks:read permission. I believe, we already require the generic read permission, so this shouldn't pose as an issue.

@BYK BYK changed the title fix(github): Don't fail when there are queued check suits fix(github): Don't fail when there are queued check suites Aug 11, 2020
Co-authored-by: Jan Michael Auer <mail@jauer.org>
Co-authored-by: Anton Ovchinnikov <anton@tonyo.info>
@BYK
Copy link
Member Author

BYK commented Aug 11, 2020

@BYK based on this patch, could we get rid of the Check Runs API and use the status and conclusion of the suites instead?

Possibly, I need to dig and debug more.

Btw. I finally found the docs for latest_check_runs_count: https://developer.github.com/changes/7/#changes-to-the-checks-rest-api (emphasis mine)

Previously, when querying a check suite, the REST API would return a key named unique_check_runs_count to indicate the quantity of check runs that had run as part of the latest push. Now, this key is called latest_check_runs_count to better indicate which types of check runs are being counted.

It indeed sounds like what we need/want here.

@BYK BYK merged commit 8abaefe into master Aug 11, 2020
@BYK BYK deleted the byk/fix/github-check-suites-pending branch August 11, 2020 10:49
BYK added a commit to getsentry/self-hosted that referenced this pull request Aug 27, 2020
BYK added a commit to getsentry/sentry that referenced this pull request Oct 14, 2020
getsentry/craft#117 is merged and released and seems to work so no need for this `sleep 10` anymore.
BYK added a commit to getsentry/sentry that referenced this pull request Oct 14, 2020
getsentry/craft#117 is merged and released and seems to work so no need for this `sleep 10` anymore.
scefali pushed a commit to getsentry/sentry that referenced this pull request Oct 15, 2020
getsentry/craft#117 is merged and released and seems to work so no need for this `sleep 10` anymore.
BYK added a commit to getsentry/sentry that referenced this pull request Oct 16, 2020
BYK added a commit to getsentry/sentry that referenced this pull request Oct 16, 2020
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.

bug: GitHub status check does not wait for builds to start

3 participants