fix(ci): fix race conditions and syntax errors - #310
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdded a reusable workflow to wait for a Docker image to become available, threaded that check into CI and deploy workflows (including a computed image tag), expanded Docker build cache sources, and fixed an indentation bug so the PR labeler runs as its own step. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as CI/Deploy Workflow
participant Waiter as wait-for-docker-image (reusable)
participant Registry as Docker Hub
participant Jobs as Dependent Jobs (lint/build/docs/react/deploy)
Caller->>Waiter: call workflow with `image` input
Waiter->>Registry: query tag metadata (tags API / docker pull)
alt image found and fresh
Registry-->>Waiter: tag_last_pushed >= workflow start
Waiter-->>Caller: return success
Caller->>Jobs: proceed with dependent jobs
else not yet available
Waiter->>Waiter: sleep, backoff, retry (up to max_attempts)
opt final fail
Waiter-->>Caller: exit 1 (fail workflow)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/deploy.yml:
- Around line 11-33: The reusable workflow calls (jobs cmake-build,
build-react-app, build-docs) incorrectly use ${{ env.image }} in their with:
inputs which is not available to reusable workflows; replace those references by
either a repository variable (e.g., create a vars entry like IMAGE and use ${{
vars.IMAGE }}) or add a setup job that emits an output (e.g., a setup job that
sets outputs.image) and pass it as ${{ needs.setup.outputs.image }} to the
cmake-build, build-react-app, and build-docs workflow calls; alternatively
inline the literal "ryanmillard/img2num-dev:latest" into each with: image if you
prefer a quick fix.
🪄 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: 162e7cdc-3fb2-4d50-a9c0-1a105281e05e
📒 Files selected for processing (2)
.github/workflows/deploy.yml.github/workflows/pr-auto-label.yml
…o-label.yml Previously unable to run as a result of this
Ryan-Millard
changed the title
Owner
Author
|
I just deleted the image and the cache to test the CI on this PR again. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/deploy.yml (1)
24-39:⚠️ Potential issue | 🔴 Critical
envcontext is still not allowed in reusable workflowwith:inputs — lines 24, 32, and 39 will evaluate to an empty string.actionlint continues to flag these as errors. GitHub Actions only exposes
github,inputs,matrix,needs,strategy, andvarsinjobs.<job>.with, so${{ env.image }}resolves to""and each called workflow receives an emptyimageinput. This contradicts the PR's stated goal of fixing the deploy workflow.Replace with either
${{ vars.IMAGE }}(simplest), a setup job that outputs the value, or the literalryanmillard/img2num-dev:main.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 24 - 39, The reusable-workflow inputs are using `${{ env.image }}` which is not available in `jobs.<job>.with` and therefore passes an empty string; update the `with:` blocks for the build-react-app and build-docs jobs (the `image:` inputs passed to the reusable workflows used by the `build-react-app` and `build-docs` jobs) to supply a valid value — replace `${{ env.image }}` with `${{ vars.IMAGE }}` (preferred), or alternatively generate the image string in a prior setup job and pass it via `needs.<job>.outputs.<name>`, or hardcode `ryanmillard/img2num-dev:main` so the called workflows receive a non-empty `image` input.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
13-91: Consider deduplicating the 5× repeated image-tag ternary.The same
github.event_name == 'pull_request' && format(...) || 'ryanmillard/img2num-dev:main'expression is copy-pasted inwait-for-image,lint.container,cmake-build,build-react-app, andbuild-docs. If the tag scheme ever changes (as it did in this PR,latest→main), all five sites must stay in sync, which is error-prone.Options:
- Expose the computed tag as an output of
wait-for-image(or a tinysetupjob) and reference${{ needs.wait-for-image.outputs.image }}everywhere. Note: this won't work forlint.container.imagedirectly ifcontaineris evaluated beforeneeds, so verify behavior —needscontext is allowed underjobs.<job>.container.- Define
vars.IMAGE_MAINin repo settings and use${{ github.event_name == 'pull_request' && format('...:pr-{0}', github.event.pull_request.number) || vars.IMAGE_MAIN }}— still duplicated, but a single source of truth for the base name.Non-blocking; current form is correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 13 - 91, The workflow duplicates the image selection expression across jobs (wait-for-image, lint.container, cmake-build, build-react-app, build-docs); consolidate by computing the image once (e.g., in the wait-for-image job or a dedicated setup job) and exposing it as an output (reference via needs.wait-for-image.outputs.image) or by defining a repo/workflow-level var (e.g., vars.IMAGE_MAIN) and using that in a single conditional expression; update each job’s image/container.image to reference the computed output/var and remove the repeated ternary to keep a single source of truth.
🤖 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/wait-for-docker-image.yml:
- Around line 49-56: The retry loop incorrectly doubles DELAY before the first
sleep and also sleeps after the final attempt; update the loop that pulls IMAGE
so that initial_delay_seconds (DELAY) is used for the first sleep by moving the
DELAY=$((DELAY * 2)) operation to after the sleep, and avoid sleeping on the
last iteration by only sleeping when the current index i is less than
MAX_ATTEMPTS (i.e., skip sleep when i == MAX_ATTEMPTS); ensure the docker pull
check (docker pull "$IMAGE" && exit 0) remains at the top of the loop so
successful pulls still exit immediately.
---
Duplicate comments:
In @.github/workflows/deploy.yml:
- Around line 24-39: The reusable-workflow inputs are using `${{ env.image }}`
which is not available in `jobs.<job>.with` and therefore passes an empty
string; update the `with:` blocks for the build-react-app and build-docs jobs
(the `image:` inputs passed to the reusable workflows used by the
`build-react-app` and `build-docs` jobs) to supply a valid value — replace `${{
env.image }}` with `${{ vars.IMAGE }}` (preferred), or alternatively generate
the image string in a prior setup job and pass it via
`needs.<job>.outputs.<name>`, or hardcode `ryanmillard/img2num-dev:main` so the
called workflows receive a non-empty `image` input.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 13-91: The workflow duplicates the image selection expression
across jobs (wait-for-image, lint.container, cmake-build, build-react-app,
build-docs); consolidate by computing the image once (e.g., in the
wait-for-image job or a dedicated setup job) and exposing it as an output
(reference via needs.wait-for-image.outputs.image) or by defining a
repo/workflow-level var (e.g., vars.IMAGE_MAIN) and using that in a single
conditional expression; update each job’s image/container.image to reference the
computed output/var and remove the repeated ternary to keep a single source of
truth.
🪄 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: 789707d4-b4ac-4920-8101-3045037ceb04
📒 Files selected for processing (5)
.github/workflows/ci.yml.github/workflows/deploy.yml.github/workflows/docker-ci.yml.github/workflows/pr-auto-label.yml.github/workflows/wait-for-docker-image.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pr-auto-label.yml
… wait-for-docker-image.yml
* This is smarter and ensures there was a recent Docker Hub push before exiting with a positive result
Owner
Author
|
I removed the docker image again |
Contributor
🐳 Docker image built successfully!Image
Run it locally:IMG2NUM_IMAGE=ryanmillard/img2num-dev:pr-310 ./img2num sh |
Owner
Author
|
The cleanup part works on closes. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/deploy.yml (1)
26-29:⚠️ Potential issue | 🔴 CriticalUse the setup job output through
needs, notsteps.These reusable workflow calls run as separate jobs, so
steps.set.outputs.imageis out of scope here. Since each job already depends onsetup, pass${{ needs.setup.outputs.image }}instead.🐛 Proposed fix
cmake-build: name: Build WASM needs: [wait-for-image, setup] uses: ./.github/workflows/cmake-build.yml with: - image: ${{ steps.set.outputs.image }} + image: ${{ needs.setup.outputs.image }} build-react-app: name: Build React App uses: ./.github/workflows/build-react-app.yml needs: [cmake-build, wait-for-image, setup] with: wasm-artifacts-path: packages/js/build-wasm/ - image: ${{ steps.set.outputs.image }} + image: ${{ needs.setup.outputs.image }} build-docs: name: Build Documentation Site needs: [wait-for-image, setup] uses: ./.github/workflows/build-docs.yml with: - image: ${{ steps.set.outputs.image }} + image: ${{ needs.setup.outputs.image }}Verify with:
#!/bin/bash set -euo pipefail # Expect no matches after the fix. rg -nP '\bimage:\s*\$\{\{\s*steps\.set\.outputs\.image\s*\}\}' .github/workflows/deploy.yml || true # If available, confirm GitHub Actions context validation. if command -v actionlint >/dev/null 2>&1; then actionlint .github/workflows/deploy.yml fiAlso applies to: 34-37, 41-44
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy.yml around lines 26 - 29, The workflow incorrectly references a step output (steps.set.outputs.image) which is out of scope for a reusable workflow job; update the call that uses ./.github/workflows/cmake-build.yml to consume the output from the setup job via the needs context (replace image: ${{ steps.set.outputs.image }} with image: ${{ needs.setup.outputs.image }}), and apply the same replacement for all other occurrences (lines referenced around the same block such as the other image: usages); ensure the job lists setup in needs so needs.setup.outputs.image is available when invoking the reusable workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/deploy.yml:
- Around line 26-29: The workflow incorrectly references a step output
(steps.set.outputs.image) which is out of scope for a reusable workflow job;
update the call that uses ./.github/workflows/cmake-build.yml to consume the
output from the setup job via the needs context (replace image: ${{
steps.set.outputs.image }} with image: ${{ needs.setup.outputs.image }}), and
apply the same replacement for all other occurrences (lines referenced around
the same block such as the other image: usages); ensure the job lists setup in
needs so needs.setup.outputs.image is available when invoking the reusable
workflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ad8acb8-74d2-4e0c-9440-b71f8706c012
📒 Files selected for processing (2)
.github/workflows/deploy.yml.github/workflows/wait-for-docker-image.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/wait-for-docker-image.yml
Ryan-Millard
added a commit
that referenced
this pull request
Apr 18, 2026
* fix(ci): fix omitted image inputs in deploy.yml and indents in pr-auto-label.yml
Previously unable to run as a result of this
* ci(docker-ci): use main's build cache as well in case other build cache doesn't exist
* ci(ci.yml): switch to main from latest docker image tag
* fix(ci): wait for Docker image before running dependent jobs
This is smarter and ensures there was a recent Docker Hub push before exiting with a positive result
This was referenced Apr 18, 2026
This was referenced May 28, 2026
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
2 participants
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.
What was changed & why
The deploy and label workflows failed to run because of syntax errors and omitted inputs - my fault in #308. This fixes that.
Fixes: none
Changes
Just syntax error fixes.
Testing & Verification
N/A - this is a YOLO thing.
Additional Resources
Summary by CodeRabbit
Bug Fixes
Chores
New Features