feat: push PR images to GHCR when a custom label is provided #6174
Conversation
Signed-off-by: Alexander Dobrodey <8377544+ADobrodey@users.noreply.github.com>
ADobrodey
requested review from
akaladarshi and
hanabi1224
and removed request for
a team
|
|
WalkthroughAdds a new GitHub Actions workflow at Changes
Sequence Diagram(s)sequenceDiagram
participant Maintainer as Maintainer
participant GH as GitHub Actions
participant Repo as Repository
participant Buildx as Docker Buildx
participant GHCR as GHCR
Note over Maintainer,GH: Manual trigger (workflow_dispatch) with `git_ref`
Maintainer->>GH: trigger workflow
GH->>Repo: checkout specified ref
GH->>Buildx: setup buildx
GH->>GHCR: authenticate using GITHUB_TOKEN
GH->>GH: generate metadata (ghcr.io/chainsafe/forest, date/sha tags)
GH->>Buildx: build fat-image (linux/amd64)
Buildx->>GHCR: push image with tags & labels
GH->>Maintainer: complete (success/failure)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/docker.yml (1)
193-193: Augmented push conditions now support custom PR labels—functional but could be more explicit.The changes correctly enable pushing images when specific PR labels (
push-fat-to-ghcr,push-slim-to-ghcr) are applied. The logic is sound: the fat and slim image steps will now push to GHCR on main/tags or when the corresponding label is present on a PR. The platforms logic (line 195, 222) remains unchanged, so PR builds will still only useamd64, which is consistent with the available artifacts.However, the PR label checks (
contains(github.event.pull_request.labels.*.name, ...)) access event properties that may not exist on push events. This relies on short-circuit evaluation of the||operator to avoid evaluating the third condition when the first two are true. While this should work in practice, it's fragile.For clarity and robustness, consider guarding the PR label check with an explicit event type check:
- push: ${{ github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/v') || contains(github.event.pull_request.labels.*.name, 'push-fat-to-ghcr') }} + push: ${{ github.ref == 'refs/heads/main' || startsWith(github.ref, 'refs/tags/v') || (github.event_name == 'pull_request' && contains(github.event.pull_request.labels.*.name, 'push-fat-to-ghcr')) }}Apply the same fix to line 220 for the slim image.
Also applies to: 220-220
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/docker.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: tests-release
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: tests
- GitHub Check: All lint checks
- GitHub Check: Build Ubuntu
- GitHub Check: Build forest binaries on Linux AMD64
LesnyRumcajs
left a comment
LesnyRumcajs
left a comment
There was a problem hiding this comment.
As mentioned, PR branches must not overwrite the edge tag.
Signed-off-by: Alexander Dobrodey <8377544+ADobrodey@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/docker.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/docker.yml
203-203: property "output" is not defined in object type {conclusion: string; outcome: string; outputs: {annotations: string; bake-file: string; bake-file-annotations: string; bake-file-labels: string; bake-file-tags: string; json: string; labels: string; tags: string; version: string}}
(expression)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: All lint checks
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: tests-release
- GitHub Check: tests
🔇 Additional comments (2)
.github/workflows/docker.yml (2)
186-197: Step design looks sound with proper label gating.The
metafatprstep correctly uses theif:condition to execute only when thepush-fat-to-ghcrlabel is present. The fallback tosteps.metafat.outputs.tagson line 203 (once the typo is fixed) ensures the workflow remains functional for non-labeled PRs.
225-234: Verify slim image label support is intentional.The PR objectives mention two labels (
push-slim-to-ghcrandpush-fat-to-ghcr), but the slim image build (lines 225–234) lacks a corresponding conditional push or metadata step like the fat image has. The slim image push condition on line 232 only checks for main branch or tag refs, not the label.Is the omission of
push-slim-to-ghcrsupport intentional, or should lines 225–234 mirror the fat image pattern with a PR-specific metadata step and label-based push condition?
…r workflow Signed-off-by: Alexander Dobrodey <8377544+ADobrodey@users.noreply.github.com>
|
Instead of triggering by tag, could we make it only be manually triggered by specifying branch and tag name? So it's on demand and decoupled from PR. (like https://github.com/ChainSafe/forest/actions/workflows/docker-latest-tag.yml or https://github.com/ChainSafe/forest/actions/workflows/lotus-devnet-publish.yml) |
|
|
I think @hanabi1224 idea is quite a good one and definitely more flexible. So it'd be basically a workflow dispatch with some inputs that would build the image on anything.
I'd recommend specifying @ADobrodey what do you think? |
…ker image based on inputs Signed-off-by: Alexander Dobrodey <8377544+ADobrodey@users.noreply.github.com>
…r workflows Signed-off-by: Alexander Dobrodey <8377544+ADobrodey@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/docker-dev.yml (1)
6-16: Clarify mutual exclusion between branch and commit inputs.The description states these inputs "Conflict with" each other, but there's no validation enforcing mutual exclusion. A user could provide both values, though the condition on line 24 only checks if at least one is non-empty.
Add explicit input validation or clarify the expected behavior when both inputs are provided. Consider adding a comment or using a different condition pattern to make the mutual exclusivity requirement more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/docker-dev.yml(1 hunks).github/workflows/docker.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/docker.yml
🧰 Additional context used
🪛 GitHub Actions: Script linters
.github/workflows/docker-dev.yml
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build MacOS
- GitHub Check: Build Ubuntu
- GitHub Check: All lint checks
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
🔇 Additional comments (2)
.github/workflows/docker-dev.yml (2)
42-50: Verify scope of label coverage against PR objectives.Per the PR objectives, support is being added for two custom labels:
push-slim-to-ghcrandpush-fat-to-ghcr. This workflow implements only thepush-fat-to-ghcrvariant (targeting thefat-imageDocker target).Confirm whether a separate workflow for
push-slim-to-ghcrshould also be added, or if the slim variant is being handled elsewhere.
27-60: Workflow structure and implementation are sound.The overall workflow is well-structured: concurrency control prevents duplicate runs, Docker Buildx is properly configured, GHCR authentication is correct, and metadata generation with date/SHA/flavor tagging is appropriate for a dev image. The amd64-only platform restriction is reasonable for CI efficiency.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
.github/workflows/docker-dev.yml (1)
1-61: Previous review flagged Prettier formatting failure—verify and fix before merge.A prior review comment noted that Prettier formatting checks failed on this file. This issue should be resolved before the PR is merged. Prettier may enforce specific spacing, indentation, or line-length requirements for YAML.
To fix locally and verify:
#!/bin/bash # Run Prettier on the workflow file to auto-fix formatting prettier --write .github/workflows/docker-dev.yml # Verify the changes git diff .github/workflows/docker-dev.yml
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/docker-dev.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build Ubuntu
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: tests-release
- GitHub Check: tests
- GitHub Check: All lint checks
🔇 Additional comments (2)
.github/workflows/docker-dev.yml (2)
27-40: Checkout, Docker setup, and registry login look good.The fallback logic in the checkout step correctly handles both branch and commit inputs. Using
secrets.GITHUB_TOKENfor GHCR authentication is the standard approach and appropriate here.
42-61: Docker metadata generation and build steps are well-structured.The tag format
YYYY-MM-DD-{{sha}}-fatprovides clear traceability, the fat-image target aligns with PR objectives, and the single-platform (amd64) build is a reasonable optimization for dev workflows. Thelatest=falsesetting is appropriate for pre-release dev images.
…low_dispatch Signed-off-by: Alexander Dobrodey <8377544+ADobrodey@users.noreply.github.com>
…ker-dev Signed-off-by: Alexander Dobrodey <8377544+ADobrodey@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/docker-dev.yml (1)
35-43: Update outdated step name comment.Line 35 refers to "Docker Meta fat for PRs only" but this workflow is now a manual
workflow_dispatchtrigger, not PR-specific. The comment is a leftover from earlier iterations.Update the step name to reflect the current design:
- - name: Docker Meta fat for PRs only + - name: Docker Meta fat image
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/docker-dev.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: All lint checks
- GitHub Check: Build forest binaries on Linux AMD64
- GitHub Check: Build MacOS
- GitHub Check: cargo-publish-dry-run
- GitHub Check: Build Ubuntu
- GitHub Check: tests-release
- GitHub Check: tests
🔇 Additional comments (1)
.github/workflows/docker-dev.yml (1)
1-53: Workflow structure and Docker target verified successfully.The workflow correctly references the
fat-imagebuild target, which is properly defined in the Dockerfile at line 58. All multi-stage build targets are present:build-env,slim-image, andfat-image. The workflow implementation with manualworkflow_dispatchtriggering, proper concurrency handling, and correct platform/target configuration is sound. Ready to merge.
| username: ${{ github.actor }} | ||
| password: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
||
| - name: Docker Meta fat for PRs only |
There was a problem hiding this comment.
nit: Not for PRs anymore
Summary of changes
Introduction of custom PR label to allow push to GHCR pre-release images
Changes introduced in this pull request:
Reference issue to close (if applicable)
Closes
Other information and links
Change checklist
Summary by CodeRabbit