chore(ci): Enable S3 caching for interop #1193
Conversation
| s3-cache-bucket: ${{ vars.S3_LIBP2P_BUILD_CACHE_BUCKET_NAME }} | ||
| s3-access-key-id: ${{ vars.S3_LIBP2P_BUILD_CACHE_AWS_ACCESS_KEY_ID }} | ||
| s3-secret-access-key: ${{ secrets.S3_LIBP2P_BUILD_CACHE_AWS_SECRET_ACCESS_KEY }} | ||
| aws-region: ${{ vars.S3_LIBP2P_BUILD_CACHE_AWS_REGION }} |
There was a problem hiding this comment.
would it be possible to avoid the duplication?
There was a problem hiding this comment.
Which duplication?
There was a problem hiding this comment.
Oh, in both calls, you mean. Not that I know.
There was a problem hiding this comment.
something like this might work
name: Interoperability Tests
on:
pull_request:
merge_group:
push:
branches:
- master
workflow_dispatch:
concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true
jobs:
run-interop-tests:
runs-on: ubuntu-22.04
strategy:
matrix:
test-type:
- transport-interop
- hole-punching-interop
name: Run ${{ matrix.test-type }} Tests
steps:
- name: Checkout Code
uses: actions/checkout@v4
- name: Setup Docker Buildx
uses: docker/setup-buildx-action@v3
- name: Build Docker Image
run: |
docker buildx build --load -t nim-libp2p-head -f tests/${{ matrix.test-type }}/Dockerfile .
- name: Run Tests
uses: libp2p/test-plans/.github/actions/run-${{ matrix.test-type == 'transport-interop' && 'transport-interop-test' || 'interop-hole-punch-test' }}@master
with:
test-filter: nim-libp2p-head
extra-versions: ${{ github.workspace }}/tests/${{ matrix.test-type }}/version.json
s3-cache-bucket: ${{ vars.S3_LIBP2P_BUILD_CACHE_BUCKET_NAME }}
s3-access-key-id: ${{ vars.S3_LIBP2P_BUILD_CACHE_AWS_ACCESS_KEY_ID }}
s3-secret-access-key: ${{ secrets.S3_LIBP2P_BUILD_CACHE_AWS_SECRET_ACCESS_KEY }}
aws-region: ${{ vars.S3_LIBP2P_BUILD_CACHE_AWS_REGION }}
There was a problem hiding this comment.
I personally feel it's clearer the other way, so for me the tradeoff is not worth.
If you still want to go with it please confirm and I'll make the change.
There was a problem hiding this comment.
I kinda agree with @AlejandroCabeza here. Sometimes, trying to remove duplicates at all cost is detrimental to understanding the code.
There was a problem hiding this comment.
Particularly, I don't think this code is much harder to understand at all. Both jobs are a duplication of each other and it's not a good practice.
There was a problem hiding this comment.
I'll wait for an agreement on this one.
There was a problem hiding this comment.
To be honest, it's not a very strong feeling on my part, we can change if Diego is adamant about it.
|
I believe this isn't necessary anymore |
.github/workflows/interop.yml
Outdated
| runs-on: ubuntu-22.04 | ||
| if: success() || failure() |
There was a problem hiding this comment.
Changing this isn't the purpose of this PR
|
Unfortunately, it's not possible to do that as it seems that |
|
@lchenut please approve it again as my approval isn't considered. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
added test wrt subscribe and unsubscribe added tests/pubsub/testgossipinternal2 file linters feat: rendezvous refactor (#1183) Hello! This PR aim to refactor rendezvous code so that it is easier to impl. Waku rdv strategy. The hardcoded min and max TTL were out of range with what we needed and specifying which peers to interact with is also needed since Waku deals with peers on multiple separate shards. I tried to keep the changes to a minimum, specifically I did not change the name of any public procs which result in less than descriptive names in some cases. I also wanted to return results instead of raising exceptions but didn't. Would it be acceptable to do so? Please advise on best practices, thank you. --------- Co-authored-by: Ludovic Chenut <[email protected]> refactor and suite name refactor chore(ci): Enable S3 caching for interop (#1193) - Adds our S3 bucket for caching docker images as Protocol Labs shut down their shared one. - Remove the free disk space workaround that prevented the jobs from failing for using too much space for the images. --------- Co-authored-by: diegomrsantos <[email protected]> PR review comment changes
Closes: #1173