-
Notifications
You must be signed in to change notification settings - Fork 284
release docker image weekly for e2e test #1268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
📝 WalkthroughWalkthroughAdds a new GitHub Actions workflow at Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Scheduler as GitHub Actions
participant Runner as ubuntu-latest
participant Buildx as Docker Buildx
participant Registry as Docker Hub
Scheduler->>Runner: scheduled / workflow_dispatch trigger
Runner->>Runner: checkout repo
Runner->>Runner: setup QEMU
Runner->>Buildx: setup Buildx builder
Runner->>Runner: run docker/metadata to produce tags/labels
Runner->>Registry: docker login using secrets
Runner->>Buildx: build image with metadata labels/tags
Buildx->>Registry: push image (scrolltech/l2geth:e2e-test, ...)
Registry-->>Runner: push result
Runner-->>Scheduler: job completes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/docker-e2e.yaml:
- Around line 11-15: The workflow job "build-and-push" currently sets
permissions to an empty object which prevents actions/checkout@v4 from
authenticating; update the job's permissions to include at least "contents:
read" so the GITHUB_TOKEN can fetch the repository (i.e., change permissions
from {} to include contents: read), ensuring actions/checkout@v4 can succeed
during the run.
🧹 Nitpick comments (3)
.github/workflows/docker-e2e.yaml (3)
3-9: Addconcurrencyto avoid overlapping weekly/manual runs overwriting the samee2e-testtag.
Since you always pushe2e-test, concurrent runs can race and produce confusing results.Proposed fix
on: @@ workflow_dispatch: +concurrency: + group: docker-e2e-weekly + cancel-in-progress: true
30-40: Publish an immutable tag in addition toe2e-test(debuggability / rollback).
Right now you only publish a mutable tag; adding asha(and/or date) tag makes it much easier to correlate failures.Proposed fix
- name: Extract docker metadata @@ # Force the tag to be 'e2e-test' tags: | type=raw,value=e2e-test + type=sha,format=short flavor: | latest=false
47-57: Consider enabling GHA cache with your separate scope (and adjust token perms if needed).
You already scoped caching to avoid production cache pollution, but it’s commented out—weekly builds may be unnecessarily slow. If you enabletype=ghacaching, verify whether you need to addactions: write(or broader) topermissions.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/docker-e2e.yaml
🧰 Additional context used
🪛 GitHub Check: zizmor
.github/workflows/docker-e2e.yaml
[failure] 18-18:
unpinned action reference
⏰ 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). (3)
- GitHub Check: test
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
.github/workflows/docker-e2e.yaml (1)
17-21: Theactions/checkoutaction is already pinned by commit SHA in this file:uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2All actions in the workflow are already SHA-pinned, contrary to the review comment's assertion. No change is needed for supply-chain hardening at this step.
Note: The pinned commit (v4.2.2) is outdated compared to the latest v4.3.1 release (commit SHA
34e114876b0b11c390a56381ad16ebd13914f8d5), but that's a separate maintenance concern unrelated to the security claim in this review.Likely an incorrect or invalid review comment.
| build-and-push: | ||
| runs-on: | ||
| group: scroll-reth-runner-group | ||
| permissions: {} | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
# Read the full workflow file to understand context and checkout usage
cat -n .github/workflows/docker-e2e.yamlRepository: scroll-tech/go-ethereum
Length of output: 2369
🏁 Script executed:
# Check if this is a public or private repository and if it has special features
git config --local --list | grep -E "url|remote"Repository: scroll-tech/go-ethereum
Length of output: 487
🏁 Script executed:
# Search for any GitHub runner group configurations and understand the setup
fd -t f "*.yaml" ".github/" -exec grep -l "scroll-reth-runner-group" {} \;Repository: scroll-tech/go-ethereum
Length of output: 571
🌐 Web query:
actions/checkout v4 permissions required GITHUB_TOKEN contents:read
💡 Result:
Yes — actions/checkout v4 expects the GITHUB_TOKEN to have at least contents: read. The checkout README recommends setting:
permissions:
contents: read
(Or provide an alternate token/ssh-key via the action inputs.) [1][2]
Sources:
[1] actions/checkout README (checkout v4).
[2] GitHub Docs — controlling permissions for the GITHUB_TOKEN.
🏁 Script executed:
# Search for any issues or error logs related to checkout in this workflow
# Check if workflow has ever failed
git log --oneline --all --grep="checkout" --since="2024-01-01" | head -20Repository: scroll-tech/go-ethereum
Length of output: 164
🏁 Script executed:
# Look for any workflow run artifacts or error messages
# Check .github for any other references to permissions or checkout
rg -A 3 -B 3 "permissions" .github/Repository: scroll-tech/go-ethereum
Length of output: 5226
Add contents: read permission for checkout to function.
actions/checkout@v4 requires the GITHUB_TOKEN to have at least contents: read scope. With permissions: {}, the action will fail to authenticate and retrieve the repository code.
Proposed fix
jobs:
build-and-push:
runs-on:
group: scroll-reth-runner-group
- permissions: {}
+ permissions:
+ contents: read📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| build-and-push: | |
| runs-on: | |
| group: scroll-reth-runner-group | |
| permissions: {} | |
| build-and-push: | |
| runs-on: | |
| group: scroll-reth-runner-group | |
| permissions: | |
| contents: read |
🤖 Prompt for AI Agents
In @.github/workflows/docker-e2e.yaml around lines 11 - 15, The workflow job
"build-and-push" currently sets permissions to an empty object which prevents
actions/checkout@v4 from authenticating; update the job's permissions to include
at least "contents: read" so the GITHUB_TOKEN can fetch the repository (i.e.,
change permissions from {} to include contents: read), ensuring
actions/checkout@v4 can succeed during the run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/docker-e2e.yaml (2)
10-14: Consider adding timeout and concurrency controls.The job lacks a
timeout-minutessetting, which could lead to hung jobs consuming runner resources. Also, without concurrency control, overlapping runs (e.g., manual trigger during a scheduled run) could cause conflicts.💡 Suggested improvements
jobs: build-and-push: runs-on: group: scroll-reth-runner-group permissions: {} + timeout-minutes: 30 + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: trueNote: Add
concurrencyat the workflow level (beforejobs:) or at the job level depending on preference.
47-57: Consider enabling the build cache.The cache configuration is commented out, which will result in full rebuilds every time. Since the comment indicates a separate cache scope (
e2e) to avoid polluting production builds, enabling it would significantly speed up weekly builds without affecting production.💡 Enable cache with separate scope
tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} # Use a separate cache scope to avoid polluting the production build cache - # cache-from: type=gha,scope=${{ github.workflow }}-e2e - # cache-to: type=gha,scope=${{ github.workflow }}-e2e + cache-from: type=gha,scope=${{ github.workflow }}-e2e + cache-to: type=gha,scope=${{ github.workflow }}-e2e,mode=max
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/docker-e2e.yaml
⏰ 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). (3)
- GitHub Check: semgrep/ci
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
.github/workflows/docker-e2e.yaml (5)
1-8: LGTM!The trigger configuration is appropriate—scheduled weekly on Fridays at 00:00 UTC with manual dispatch for testing. The pinned schedule and manual override provide good flexibility.
16-20: LGTM!Good security practices: pinned action SHA and
persist-credentials: falseto avoid leaking credentials.
22-28: Clarify intent: QEMU setup without multi-platform build.QEMU is set up (typically used for cross-platform builds), but the
build-push-actiondoesn't specify aplatformsparameter. This means only the native runner architecture will be built.If multi-arch is intended, add
platformsto the build step. If only single-arch is needed, QEMU setup can be removed to speed up the workflow.💡 If multi-arch is intended
Add to the build-push step:
platforms: linux/amd64,linux/arm64
30-39: LGTM!Metadata configuration correctly forces the
e2e-testtag and explicitly disableslatestto avoid accidental overwrites.
41-45: LGTM!Proper use of repository secrets for Docker Hub authentication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/docker-e2e.yaml (1)
47-57: Consider specifying target platforms.QEMU is set up (line 22-23), but no
platformsargument is specified in the build-push action. The build will default to the runner's native architecture. If multi-platform images (e.g.,linux/amd64,linux/arm64) are intended, add theplatformsinput. Otherwise, the QEMU setup step may be unnecessary.Example for multi-platform build
- name: Build and push docker image uses: docker/build-push-action@263435318d21b8e681c14492fe198d362a7d2c83 # v6.18.0 with: context: . file: Dockerfile push: true + platforms: linux/amd64,linux/arm64 tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/docker-e2e.yaml
⏰ 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). (3)
- GitHub Check: test
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
.github/workflows/docker-e2e.yaml (4)
1-14: LGTM on workflow configuration.The scheduled trigger, manual dispatch option, and restrictive permissions (
permissions: {}) follow security best practices. Using pinned SHA references for actions throughout the workflow is excellent for supply chain security.
16-28: Good security practices in setup steps.Using
persist-credentials: falsefor checkout and pinned SHA references for all actions are solid security measures.
30-39: Verify the correct image repository name.The AI summary indicates the image should be
scrolltech/rollup-node, but the workflow specifiesscrolltech/l2geth. Please confirm which image repository is intended for this E2E workflow.
41-45: Ensure Docker Hub secrets are configured.The workflow depends on
DOCKERHUB_USERNAMEandDOCKERHUB_TOKENsecrets. Verify these are configured in the repository or organization settings to avoid workflow failures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.github/workflows/docker-e2e.yaml (3)
21-22: QEMU setup may be unnecessary without multi-platform builds.QEMU is typically used for cross-platform Docker builds, but the
build-push-actionstep doesn't specify aplatformsparameter. If only building for the default platform (linux/amd64), this step adds ~10-15 seconds of overhead without benefit.Consider either:
- Removing this step if single-platform builds are sufficient for e2e testing.
- Adding
platforms: linux/amd64,linux/arm64to the build step if multi-arch is needed.
40-44: Minor: Inconsistent comment formatting.Line 41 uses
#v3.4.0while other action references use# v3.x.x(with a space after#).Proposed fix
- name: Login to Docker Hub - uses: docker/login-action@74a5d142397b4f367a81961eba4e8cd7edddf772 #v3.4.0 + uses: docker/login-action@74a5d142397b4f367a81961eba4e8cd7edddf772 # v3.4.0 with:
46-56: Consider enabling build cache for faster CI runs.The cache configuration is commented out. For weekly scheduled builds, caching may provide limited benefit, but enabling it would significantly speed up manual
workflow_dispatchruns during iterative testing.If there are concerns about cache isolation, the commented scope approach (
scope=${{ github.workflow }}-e2e) already addresses this by keeping e2e cache separate from production builds.Proposed change to enable caching
tags: ${{ steps.meta.outputs.tags }} labels: ${{ steps.meta.outputs.labels }} # Use a separate cache scope to avoid polluting the production build cache - # cache-from: type=gha,scope=${{ github.workflow }}-e2e - # cache-to: type=gha,scope=${{ github.workflow }}-e2e + cache-from: type=gha,scope=${{ github.workflow }}-e2e + cache-to: type=gha,scope=${{ github.workflow }}-e2e,mode=max
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/docker-e2e.yaml
⏰ 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). (3)
- GitHub Check: semgrep/ci
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
.github/workflows/docker-e2e.yaml (1)
1-13: Well-structured workflow with good security defaults.The workflow is properly configured with:
- Pinned action SHAs with version comments for reproducibility and security.
- Minimal permissions (
permissions: {}) following the principle of least privilege.- Scheduled cron trigger plus manual dispatch for flexibility.
1. Purpose or design rationale of this PR
...
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
Note: This release contains no user-facing changes.
✏️ Tip: You can customize this high-level summary in your review settings.