Conversation
|
/describe |
|
✅ PR description has been generated and updated! |
33db075 to
bcfe3af
Compare
|
/review |
|
I do not see the check :/ |
There was a problem hiding this comment.
Review Summary
Assessment: 🟡 NEEDS_ATTENTION
This PR adds GitHub Checks API integration to surface workflow progress as check runs on PRs. The implementation is sound overall, but there are 4 potential issues related to error handling that should be addressed:
- 1 edge case where cancelled/timed-out workflows may leave orphaned
in_progresschecks - 3 instances of missing error handling in
checks.update()API calls that could leave checks in inconsistent states
All findings are medium severity and likely to occur under certain conditions (workflow cancellations, API failures, rate limiting).
Recommendations
- Wrap API calls in try-catch blocks to provide proper error logging and recovery
- Consider adding a cleanup mechanism for orphaned checks (e.g., a separate workflow that queries and closes stale
in_progresschecks) - Add error logging when check creation/update fails to aid debugging
The core functionality is well-designed with good safeguards (continue-on-error, if: always()), but the error paths need better handling.
| - name: Check out Git repository | ||
| uses: actions/checkout@d632683dd7b4114ad314bca15554477dd762a938 # v4.2.0 | ||
|
|
||
| - name: Create check run |
There was a problem hiding this comment.
Orphaned in_progress check run if workflow is cancelled or times out
While the if: always() condition on the update step ensures it runs on normal job completion, it won't execute if the workflow is cancelled at the job level or hits a hard timeout before reaching the update step. This could leave an orphaned check permanently spinning in in_progress status on the PR.
Recommendation: Consider adding a separate cleanup workflow that runs periodically to find and close stale in_progress check runs, or use GitHub's auto-timeout feature for checks (though this requires additional configuration).
.github/workflows/pr-describe.yml
Outdated
| with: | ||
| script: | | ||
| const conclusion = process.env.JOB_STATUS === 'cancelled' ? 'cancelled' : process.env.JOB_STATUS === 'success' ? 'success' : 'failure'; | ||
| await github.rest.checks.update({ |
There was a problem hiding this comment.
Missing error handling for checks.update API call
The github.rest.checks.update() call is not wrapped in try-catch. If the API fails (rate limiting, network issues, invalid check_run_id), the step fails silently and may leave the check in an inconsistent state.
Recommendation: Wrap the API call in try-catch:
try {
await github.rest.checks.update({
owner: context.repo.owner,
repo: context.repo.repo,
check_run_id: parseInt(process.env.CHECK_ID, 10),
status: 'completed',
conclusion: conclusion,
completed_at: new Date().toISOString()
});
} catch (error) {
core.warning(`Failed to update check run: ${error.message}`);
}
.github/workflows/review-pr.yml
Outdated
| with: | ||
| script: | | ||
| const conclusion = process.env.JOB_STATUS === 'cancelled' ? 'cancelled' : process.env.JOB_STATUS === 'success' ? 'success' : 'failure'; | ||
| await github.rest.checks.update({ |
There was a problem hiding this comment.
Missing error handling for checks.update API call
The github.rest.checks.update() call is not wrapped in try-catch. API failures will fail the step silently and may leave checks in inconsistent states.
Recommendation: Add try-catch error handling with logging (same pattern as pr-describe.yml suggestion).
.github/workflows/self-review-pr.yml
Outdated
| with: | ||
| script: | | ||
| const conclusion = process.env.JOB_STATUS === 'cancelled' ? 'cancelled' : process.env.JOB_STATUS === 'success' ? 'success' : 'failure'; | ||
| await github.rest.checks.update({ |
There was a problem hiding this comment.
Missing error handling for checks.update API call
The github.rest.checks.update() call lacks try-catch error handling. API failures could leave checks in an inconsistent state without proper logging.
Recommendation: Add try-catch error handling with logging (same pattern as pr-describe.yml suggestion).
Signed-off-by: Derek Misler <derek.misler@docker.com>
bcfe3af to
feb02ee
Compare
Summary
This PR adds GitHub Checks API integration to all three CI workflows so that manually triggered reviews and PR description generation are surfaced as proper check runs on the PR's commit. Previously, these workflows ran silently without any visible check status on the PR; now they create a check run at the start and update it to
success,failure, orcancelledupon completion.Changes
.github/workflows/pr-describe.yml: Addedchecks: writepermission, aCreate check runstep at job start (namedGenerate PR Description), and anUpdate check runstep (runsalways()) to finalize the check status..github/workflows/review-pr.yml: Same pattern — addedchecks: write, creates aPR Reviewcheck run using the resolvedpr-numberinput orissue.number, and updates it on completion..github/workflows/self-review-pr.yml: Same pattern asreview-pr.yml— addschecks: write, creates and finalizes aPR Reviewcheck run tied to the PR'shead_sha.How to Test
workflow_dispatch); aPR RevieworGenerate PR Descriptioncheck should appear on the PR's Checks tab showingin_progressand then resolve tosuccessorfailure.cancelledrather than hanging asin_progress.