refactor(e2e): only build zetanode once #3121
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the CI/CD workflows and Docker configurations for the Zeta node application. A new job, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
gartnera
force-pushed
the
e2e-build-once
branch
2 times, most recently
from
6d5b79f
to
bc785b3
Compare
add docker login back to reusable flow
gartnera
force-pushed
the
e2e-build-once
branch
from
bc785b3
to
b381de2
Compare
gartnera
force-pushed
the
e2e-build-once
branch
from
ee9f188
to
b220a79
Compare
gartnera
requested review from
a team,
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
contrib/localnet/orchestrator/Dockerfile.fastbuild (2)
Line range hint
7-14: Consider using COPY --chmod instead of separate RUN chmod.The current approach uses a separate RUN instruction to chmod the shell scripts. Consider using the more efficient COPY --chmod syntax to reduce the number of layers.
-COPY contrib/localnet/orchestrator/start-zetae2e.sh /work/ -COPY contrib/localnet/scripts/wait-for-ton.sh /work/ -COPY cmd/zetae2e/config/localnet.yml /work/config.yml -RUN chmod +x /work/*.sh +COPY --chmod=755 contrib/localnet/orchestrator/start-zetae2e.sh /work/ +COPY --chmod=755 contrib/localnet/scripts/wait-for-ton.sh /work/ +COPY cmd/zetae2e/config/localnet.yml /work/config.yml
Container images not found in registry - Action Required
The verification process has revealed that the specified container images are not accessible in the GitHub Container Registry:
ghcr.io/zeta-chain/ethereum-client-go:v1.10.26ghcr.io/zeta-chain/solana-docker:1.18.15This indicates a critical issue that needs immediate attention:
- The images might have been removed, renamed, or never existed in the specified registry
- The versions specified might be outdated or incorrect
- Access permissions might be incorrectly configured
Recommendation:
- Verify and update the image references to use currently available versions
- Consider pinning to specific SHA digests instead of tags for better reproducibility
- Ensure proper documentation of required container images and their versions
🔗 Analysis chain
Line range hint
1-14: Verify image dependencies and their versions.The Dockerfile relies on three different base images. Let's verify their availability and version compatibility:
- ethereum-client-go:v1.10.26
- solana-docker:1.18.15
- zetanode:latest
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and pull-ability of required container images # Test: Check if the images exist in the container registry for IMAGE in "ghcr.io/zeta-chain/ethereum-client-go:v1.10.26" "ghcr.io/zeta-chain/solana-docker:1.18.15" do gh api -H "Accept: application/vnd.oci.image.index.v1+json" "/v2/$IMAGE/manifests/latest" &>/dev/null if [ $? -eq 0 ]; then echo "✓ Image $IMAGE exists" else echo "✗ Image $IMAGE not found" fi doneLength of output: 769
Script:
#!/bin/bash # Let's check if there are any references to these image versions in other files # This might help us understand if these versions are referenced elsewhere or if there are newer versions # Search for ethereum-client-go version references echo "=== Searching for ethereum-client-go version references ===" rg -i "ethereum-client-go.*v[0-9]+" --type yaml --type dockerfile echo -e "\n=== Searching for solana-docker version references ===" rg -i "solana-docker.*[0-9]+\.[0-9]+\.[0-9]+" --type yaml --type dockerfile # Check if there's any documentation about these dependencies echo -e "\n=== Checking documentation for dependency versions ===" fd -t f "README.md|DEPENDENCIES.md|VERSION" -X cat {}Length of output: 11373
.github/workflows/reusable-e2e.yml (1)
56-56: Remove trailing whitespace.There is trailing whitespace on this line that should be removed to maintain consistent formatting.
- +🧰 Tools
🪛 yamllint
[error] 56-56: trailing spaces
(trailing-spaces)
.github/workflows/e2e.yml (2)
45-50: Enhance Docker Hub login security.The Docker Hub login is correctly restricted to trusted contexts, but consider using a more explicit condition for better maintainability.
-if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == 'zeta-chain/node' +if: | + github.event_name != 'pull_request' || + (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == 'zeta-chain/node')
304-307: Enhance error messaging in build verification.While the error handling is correct, consider adding more explicit error messages for better debugging.
result="${{ needs.build-zetanode.result }}" if [[ $result != "success" ]]; then + echo "::error::zetanode build failed with result: $result" exit 1 fiMakefile (2)
246-258: LGTM! Consider adding error handling for image pull failures.The conditional logic for building vs pulling the zetanode image is well-structured. However, consider adding error handling for the docker pull operation to gracefully fall back to building from source if the pull fails.
ifdef ZETANODE_IMAGE zetanode: @echo "Pulling zetanode image" - $(DOCKER) pull $(ZETANODE_IMAGE) - $(DOCKER) tag $(ZETANODE_IMAGE) zetanode:latest + $(DOCKER) pull $(ZETANODE_IMAGE) || \ + { echo "Failed to pull image, falling back to build"; \ + $(DOCKER) build -t zetanode --build-arg NODE_VERSION=$(NODE_VERSION) \ + --build-arg NODE_COMMIT=$(NODE_COMMIT) --target latest-runtime \ + -f ./Dockerfile-localnet .; } + $(DOCKER) tag $(ZETANODE_IMAGE) zetanode:latest || true
273-313: LGTM! Consider adding target descriptions.The E2E test targets are well-structured with appropriate dependencies on
e2e-images. Consider adding target descriptions using double-hash comments to improve maintainability.+## Runs the standard E2E test suite start-e2e-test: e2e-images @echo "--> Starting e2e test" cd contrib/localnet/ && $(DOCKER_COMPOSE) up -d +## Runs E2E tests for admin functionality start-e2e-admin-test: e2e-images @echo "--> Starting e2e admin test" export E2E_ARGS="--skip-regular --test-admin" && \ cd contrib/localnet/ && $(DOCKER_COMPOSE) --profile eth2 up -d
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
.github/workflows/e2e.yml(4 hunks).github/workflows/reusable-e2e.yml(3 hunks)Dockerfile-localnet(2 hunks)Makefile(5 hunks)contrib/localnet/orchestrator/Dockerfile.fastbuild(1 hunks)
🧰 Additional context used
🪛 actionlint
.github/workflows/e2e.yml
78-78: shellcheck reported issue in this script: SC2086:info:2:38: Double quote to prevent globbing and word splitting
(shellcheck)
78-78: shellcheck reported issue in this script: SC2086:info:4:36: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 yamllint
.github/workflows/reusable-e2e.yml
[error] 56-56: trailing spaces
(trailing-spaces)
🔇 Additional comments (7)
.github/workflows/reusable-e2e.yml (3)
22-25: Well-structured input parameter definition!
The new zetanode-image parameter is properly documented and correctly configured as a required string input, which aligns with the PR's objective of centralizing the zetanode image building process.
38-39: Correct environment variable configuration!
The ZETANODE_IMAGE environment variable is properly configured at the job level, making the pre-built image reference available to all steps within the e2e job.
Line range hint 1-96: Verify workflow integration with main e2e workflow.
Let's ensure this reusable workflow is properly referenced and that the zetanode-image parameter is correctly passed from the main workflow.
✅ Verification successful
Workflow integration verified successfully
The reusable workflow is properly integrated and used in the main e2e workflow with correct parameter passing:
- The workflow is referenced correctly in
e2e.yml - The
zetanode-imageparameter is properly passed from the build job output:zetanode-image: ${{ needs.build-zetanode.outputs.image }} - All required inputs are correctly mapped in the matrix strategy
- The workflow maintains proper job dependencies: build-zetanode → matrix-conditionals → e2e
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration with the main e2e workflow
# Expected: Find references to this reusable workflow and verify parameter passing
# Search for references to this reusable workflow
echo "Checking references to the reusable workflow:"
rg -l "uses: ./.github/workflows/reusable-e2e.yml"
# Search for zetanode-image parameter usage
echo -e "\nChecking zetanode-image parameter usage:"
rg "zetanode-image:" .github/workflows/
Length of output: 437
Script:
#!/bin/bash
# Let's verify the complete workflow chain and parameter passing
# Check the complete e2e.yml workflow to understand the flow
echo "Content of e2e.yml workflow:"
cat .github/workflows/e2e.yml
# Check if there are any other workflows that might be using this reusable workflow
echo -e "\nChecking for any other potential workflow references:"
rg -l "reusable-e2e"
Length of output: 15459
🧰 Tools
🪛 actionlint
21-21: input "runs-on" of workflow_call event has the default value "ubuntu-20.04", but it is also required. if an input is marked as required, its default value will never be used
(events)
.github/workflows/e2e.yml (1)
195-197: LGTM! Well-structured job dependencies and parameter passing.
The changes correctly ensure that:
- The e2e job waits for both the image build and matrix computation
- The zetanode image is properly passed to the reusable workflow
Also applies to: 248-248
Makefile (3)
230-231: LGTM! Good consolidation of image building targets.
The new e2e-images target effectively consolidates the building of both zetanode and orchestrator images, which helps prevent redundant builds across different test targets.
259-262: LGTM! Clean orchestrator build target.
The orchestrator build target is well-defined and uses a fastbuild Dockerfile, which aligns with the PR's goal of optimizing build times.
Line range hint 325-340: LGTM! Well-structured upgrade test targets.
The upgrade test targets correctly implement the conditional build logic and maintain proper dependencies on e2e-images. The build arguments are appropriately passed, ensuring version consistency.
gartnera
force-pushed
the
e2e-build-once
branch
from
772e180
to
3cbdb50
Compare
Description
Only build the zetanode image once per e2e matrix. Local development flow should be unchanged.
The zetanode localnet image is already quite slim so pushing is quite fast (less than 10 seconds in my tests).
Unrelated but this also give you a bit more time to add e2e labels to your PR when opening as the matrix computation is deferred a bit now.
Also unrelated but now you can rerun a failed job without having to wait for the build again.
Related to #3120. When merged, it will push
ghcr.io/zeta-chain/zetanode:developimage.Fork context tested here: #3123. Update, looks like this will never work for forks as they can never push packages (even to their own user's packages). Will workaround by just falling back to the slow build on forks I guess.
Summary by CodeRabbit
Release Notes
New Features
build-zetanodein the CI workflow to streamline the building of the Zeta node image.zetanode-imageto the reusable E2E testing workflow for improved image handling.Improvements
Bug Fixes