fix(build order): force ci to happen after docker-ci - #321
Conversation
📝 WalkthroughWalkthroughThe CI workflow is refactored from a directly-triggered workflow (via pull_request and push events) to a reusable workflow that can be invoked by other workflows. The docker-ci workflow now includes a new test job that calls this reusable ci workflow after the build job completes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/docker-build |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
29-51:⚠️ Potential issue | 🔴 Critical
github.event.pull_request.numberwill be empty in the new call context, breaking PR image resolution.When
ci.ymlwas triggered bypull_request,github.event.pull_request.numberwas populated. Now that it's invoked viaworkflow_callfromdocker-ci.yml— whose triggering events areissue_comment,push, andpull_request: closed—github.event.pull_requestis not available for theissue_commentandpushcases. In a called workflow,github.eventreflects the caller's triggering event, soPR_TAGwill resolve to the literal stringpr-(empty number), both GHCR and DH lookups will 404, and every job will silently fall back toghcr.io/ryan-millard/img2num-dev:maininstead of the freshly built PR image — defeating the whole purpose of running CI afterdocker-ci.The cleanest fix is to accept the PR number (and/or the resolved image tag) as a
workflow_callinput fromdocker-ci.yml, where it's already available vianeeds.guard.outputs.pr_number/needs.build.outputs.clean_core_tag.Proposed direction
In
ci.yml:on: workflow_call: inputs: - ready: - required: true - type: string + pr_number: + required: false + type: string + default: "" + image_tag: + required: false + type: string + default: ""- name: Set image id: set run: | MAIN_IMAGE="ghcr.io/ryan-millard/img2num-dev:main" - PR_TAG="pr-${{ github.event.pull_request.number }}" + PR_TAG="${{ inputs.image_tag != '' && inputs.image_tag || format('pr-{0}', inputs.pr_number) }}"In
docker-ci.yml'stestjob:test: needs: [guard, build] uses: ./.github/workflows/ci.yml with: - ready: True + pr_number: ${{ needs.guard.outputs.pr_number }} + image_tag: ${{ needs.build.outputs.clean_core_tag }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 29 - 51, The script uses github.event.pull_request.number to build PR_TAG (and GHCR_IMAGE/DH_IMAGE) but that field can be empty when ci.yml is invoked via workflow_call; change ci.yml to accept a workflow_call input (e.g., pr_number and/or resolved_image_tag) and use that input instead of github.event.pull_request.number to set PR_TAG and the GHCR_IMAGE/DH_IMAGE fallbacks, then update the caller (docker-ci.yml) to pass needs.guard.outputs.pr_number or needs.build.outputs.clean_core_tag into those inputs so PR image resolution works in called workflows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 3-13: Restore automatic PR and main push triggers by re-adding
pull_request (opened, synchronize, reopened) and push: branches: [main] to this
workflow's on: section instead of making it callable-only; remove the
commented-out on: block entirely; if you must keep workflow_call, also add
pull_request/push triggers so PRs run automatically. Remove the unused workflow
input named ready (and delete the corresponding with: { ready: True } usage in
docker-ci.yml). Alternatively, keep this workflow callable-only but update
docker-ci.yml to trigger on pull_request events (opened/synchronize/reopened) or
make the set-image job wait/depend on the image publish step so tests can run
automatically—adjust the set-image job dependency if chosen.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 29-51: The script uses github.event.pull_request.number to build
PR_TAG (and GHCR_IMAGE/DH_IMAGE) but that field can be empty when ci.yml is
invoked via workflow_call; change ci.yml to accept a workflow_call input (e.g.,
pr_number and/or resolved_image_tag) and use that input instead of
github.event.pull_request.number to set PR_TAG and the GHCR_IMAGE/DH_IMAGE
fallbacks, then update the caller (docker-ci.yml) to pass
needs.guard.outputs.pr_number or needs.build.outputs.clean_core_tag into those
inputs so PR image resolution works in called workflows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 169dfeb4-9abe-43b9-a183-ee36ece4a774
📒 Files selected for processing (2)
.github/workflows/ci.yml.github/workflows/docker-ci.yml
| # on: | ||
| # pull_request: | ||
| # branches: [main] | ||
| # push: | ||
| # branches: [main] | ||
| on: | ||
| pull_request: | ||
| branches: [main] | ||
| push: | ||
| branches: [main] | ||
| workflow_call: | ||
| inputs: | ||
| ready: | ||
| required: true | ||
| type: string |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# List GitHub workflows to understand the structure
ls -la .github/workflows/Repository: Ryan-Millard/Img2Num
Length of output: 963
🏁 Script executed:
#!/bin/bash
# Read the ci.yml file to verify the current state
cat -n .github/workflows/ci.ymlRepository: Ryan-Millard/Img2Num
Length of output: 3747
🏁 Script executed:
#!/bin/bash
# Read the docker-ci.yml file to verify trigger conditions
cat -n .github/workflows/docker-ci.ymlRepository: Ryan-Millard/Img2Num
Length of output: 12803
🏁 Script executed:
#!/bin/bash
# Search for references to the 'ready' input throughout the workflows
rg "ready" .github/workflows/Repository: Ryan-Millard/Img2Num
Length of output: 151
Regression confirmed: PRs no longer receive automatic CI on open/synchronize.
By removing pull_request/push triggers and making this workflow callable-only, CI now runs only when docker-ci.yml's test job invokes it. docker-ci.yml itself triggers on issue_comment, push (main/tags), and pull_request: closed — none of which fire when a contributor opens or updates a PR. The net effect is that lint, C/C++ build, React build, and docs build will not run on a PR until a maintainer manually comments /docker-build <sha>. For external contributors and normal PR review, this means no CI coverage by default, which is a meaningful quality-gate regression versus the previous behavior.
If the goal is simply "ensure the docker image is built before CI runs against it" (per the PR title), consider instead keeping pull_request triggers here but having the set-image job wait for / depend on the image publish, or gate individual jobs on image availability rather than disabling automatic triggers entirely. Alternatively, have docker-ci.yml also trigger on pull_request: [opened, synchronize, reopened] so the full guard → build → test chain runs automatically.
Additionally:
- The commented-out
on:block (Lines 3–7) should be deleted rather than left as comments. readyis declaredrequired: truebut is never referenced in any job in this file — it's a dead input. Drop it (and thewith: { ready: True }indocker-ci.yml).
Please confirm whether losing automatic PR CI (and CI on push to main) is intentional. If yes, please also confirm how main-branch regressions will be caught before release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 3 - 13, Restore automatic PR and main
push triggers by re-adding pull_request (opened, synchronize, reopened) and
push: branches: [main] to this workflow's on: section instead of making it
callable-only; remove the commented-out on: block entirely; if you must keep
workflow_call, also add pull_request/push triggers so PRs run automatically.
Remove the unused workflow input named ready (and delete the corresponding with:
{ ready: True } usage in docker-ci.yml). Alternatively, keep this workflow
callable-only but update docker-ci.yml to trigger on pull_request events
(opened/synchronize/reopened) or make the set-image job wait/depend on the image
publish step so tests can run automatically—adjust the set-image job dependency
if chosen.
|
/docker-build 36d06fc |
🐳 Docker image built successfully!Image
Run it locally:IMG2NUM_IMAGE=ghcr.io/ryan-millard/img2num-dev:pr-321 ./img2num sh |
1 participant
run docker-ci.yml first always, then ci.yml
What was changed & why
Fixes: #
Changes
Testing & Verification
Additional Resources
Summary by CodeRabbit