Report queued builds as "queued", drive one check run through its full lifecycle - #374
Merged
Merged
Conversation
GitHubStatusPush's default generators report both a build-request-queued event and the actual build start/end. Both produce state == "pending" in sendMessage() (build['complete'] is False for a queued request too), so createStatus() was creating a check run for the queued-but-not-yet-started event as well as the real one for build start -- but unlike statuses, check runs are persistent objects with their own id, so the queued-request one was never completed and sat forever as "in progress" pointing at a buildrequest page instead of a build.
…fecycle The previous commit dropped BuildRequestGenerator entirely, losing the queued-but-not-started feedback (a static dot, distinct from an actively running build) that we actually want. The real bug wasn't reporting the queued state -- it was creating a brand new check run for it instead of reusing the one for the build's later start/completion. Both the queued (BuildRequestGenerator) and started (BuildStartEndStatus- Generator) reports produce state == "pending" in the inherited sendMessage(), but their target_url differs -- a buildrequest page vs. an actual build page -- which is enough to tell "queued" from "in progress" apart. createStatus() now always looks up the existing check run by name first and PATCHes it, falling back to POST only when none exists yet, so a build's queued, started, and completed reports all update the same check run. Verified against a fake HTTP session driving createStatus() through all three stages: exactly one check run exists throughout, transitioning queued -> in_progress -> completed.
alexreinking
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/buildrequests/...page (a queued request, not an actual build).GitHubStatusPush's default generators include both aBuildRequestGenerator(fires when a build is queued, before any worker picks it up) and aBuildStartEndStatusGenerator(fires on actual start/end). Both producestate == "pending"in the inheritedsendMessage(), soGitHubAppCheckPush.createStatus()was creating a separate check run for each -- but unlike statuses (where a new post for the same context just overwrites the display), check runs are persistent objects with their own id, so the queued-request one was never touched again and sat forever stuck "in progress".createStatus()now always looks up the existing check run by name first andPATCHes it, falling back toPOSTonly when none exists yet. The queued report'starget_urlpoints at a buildrequest page rather than an actual build (getURLForBuildrequestvs.getURLForBuildproduce structurally different paths), which is enough to tell "queued" from "in progress" apart despite both arriving asstate == "pending".queued->in_progress->completedcleanly.Test plan
pre-commit run --files master/github_app_check_push.py-- all hooks passbuildbot checkconfigpassescreateStatus()directly against a fake HTTP session through all three stages (queued report with a buildrequest URL, started report with a build URL, completed report) and asserted exactly one check run existed throughout, withstatustransitioning queued -> in_progress -> completed and the rightconclusionon completion