-
Notifications
You must be signed in to change notification settings - Fork 4
[NONEVM-1712] CTF Integration with GHA Cache #12
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
Conversation
This reverts commit 3b5cade.
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.
Pull Request Overview
This PR introduces CTF integration for TON blockchain integration tests by adding a caching mechanism for MyLocalTon Docker images, which aims to speed up CI test runs.
- Added a TOML configuration for TON blockchain integration
- Created a new Go-based integration test for TON blockchain smoke testing
- Configured GitHub workflows and a composite action to pull, cache, and refresh Docker images
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| integration-tests/examples/smoke_ton.toml | New config file defining blockchain type, image URL, and service list for TON tests. |
| integration-tests/examples/ctf_test.go | Introduces an integration test including blockchain network setup and wallet operations. |
| .github/workflows/test-ctf.yml | Workflow to run integration tests along with setting up MyLocalTon images and Go environment. |
| .github/workflows/cache-mylocalton-images.yml | Workflow for scheduled and manual caching of MyLocalTon Docker images. |
| .github/actions/mylocalton-images/action.yml | Composite action to prepare cache directories, manage Docker image pulls/saves, and update the cache. |
Files not reviewed (2)
- integration-tests/.gitignore: Language not supported
- integration-tests/go.mod: Language not supported
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Check out code | ||
| uses: actions/checkout@0ad4b8fadaa221de15dcec353f45205ec38ea70b # v4.1.4 |
Copilot
AI
May 14, 2025
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.
Consider using a stable version tag (e.g., actions/checkout@v4) instead of a commit hash to make the workflow easier to maintain.
| uses: actions/checkout@0ad4b8fadaa221de15dcec353f45205ec38ea70b # v4.1.4 | |
| uses: actions/checkout@v4 |
| uses: actions/cache@v4 | ||
| with: | ||
| path: ${{ inputs.cache-dir }}/docker-images.tar | ||
| key: mylocalton-images-${{ hashFiles('${{ inputs.cache-dir }}/docker-compose.yml') }} |
Copilot
AI
May 14, 2025
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.
[nitpick] Double-check the use of nested expressions inside hashFiles; consider simplifying the expression to ensure that the correct file is hashed for cache key generation.
| key: mylocalton-images-${{ hashFiles('${{ inputs.cache-dir }}/docker-compose.yml') }} | |
| key: mylocalton-images-${{ hashFiles(compose_file_path) }} |
.github/workflows/test-ctf.yml
Outdated
| uses: ./.github/actions/mylocalton-images | ||
|
|
||
| - name: Setup Go | ||
| uses: smartcontractkit/.github/actions/setup-golang@setup-golang/v1 |
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.
Please adopt and use Nix - we will not use custom unportable GHA setups
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.
Yes, absolutely. I had copied it from another repo’s workflow and added it temporarily because the Go dependency installation was taking a long time during testing. Thanks for the feedback!
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.
fixed in 7b3cde2
| description: 'Space-separated list of Docker images' | ||
| required: false | ||
| default: >- | ||
| ghcr.io/neodix42/mylocalton-docker:latest |
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.
Please try to simplify this custom action (ideally remove it completely) by reusing existing Docker bake action which has support for caching and pulling from cache using an additional configuration file: https://twistezo.github.io/blog/posts/caching-docker
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.
I have a question about this part — please correct me if I’m wrong. I ran some tests based on the reference link you shared, but the docker-compose file we’ll be using (which will be hosted remotely and not modified) doesn’t include any build step. My understanding is that Docker Bake Action is primarily for caching build layers. Because of that, I initially failed to implement it, but I’ll give it another try. I agree that we should avoid using custom scripts.
| image = "https://raw.githubusercontent.com/neodix42/mylocalton-docker/main/docker-compose.yaml" | ||
| ton_core_services = [ |
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.
Why is this declared as an image?
Why are these services configured as ton_core_services - couldn't this be general Docker compose support, and a configuration for a list of any Docker compose services?
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.
Thanks for the feedback! I initially used the existing field image, but it has now been changed to docker_compose_file_url.
You’re right about the Docker images — it makes sense for the field names to represent general image options that can be selected within a Docker Compose setup
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.
replaced with docker_compose_file_url / docker_compose_services. fixed in f9b8858
| [blockchain_a] | ||
| type = "ton" |
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.
[blockchain_a]
type = "ton"
What is blockchain_a?
What is type = "ton" used for, if we also configure the docker-compose.yaml file?
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.
These are CTF configuration fields.
iiuc, blockchain_a indicates the first blockchain involved in this test setup
Here's the other test configuration involving 2 chains -- with blockchain_a and blockchain_b
https://github.com/smartcontractkit/chainlink-testing-framework/blob/96212dfb45f083f6358e7a85f4e0e85d35353168/framework/examples/myproject/chaos/chaos_reorg.toml#L7
type = "ton" is another input field used for getting the right blockchain handler
https://github.com/smartcontractkit/chainlink-testing-framework/blob/96212dfb45f083f6358e7a85f4e0e85d35353168/framework/components/blockchain/blockchain.go#L82
I’m checking how we can move this chain-specific test network implementation into chainlink-ton, then probably other integration projects would import it when they need to run tests related to TON, since these fields will likely be needed for them as well?
|
Closing this PR to switch to a different E2E pattern. #18 The image caching and test setup can still be reused. |
This PR introduces CTF integration in TON blockchain integration tests with GHA Cache NONEVM-1712
Primary Changes
Others
Requires