Skip to content
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.

Sign up for GitHub

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

chore(ci): Enable S3 caching for interop #1193

Merged
merged 8 commits into from
Sep 26, 2024
Merged

chore(ci): Enable S3 caching for interop #1193

Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
65d9acc
Pass S3 variables for caching into interop workflow.
AlejandroCabeza Sep 12, 2024
50c93e4
Remove duplicated keyword
AlejandroCabeza Sep 12, 2024
2c5aee1
Deduplicate args.
AlejandroCabeza Sep 25, 2024
0d2afe0
Merge branch 'master' into feature/interop-caching
diegomrsantos Sep 25, 2024
a26aa1a
fix simplify yml
diegomrsantos Sep 25, 2024
d8b6b64
Revert "fix simplify yml"
diegomrsantos Sep 25, 2024
524da61
Revert "Deduplicate args."
diegomrsantos Sep 25, 2024
5a06f86
remove free disk step
diegomrsantos Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 26 additions & 27 deletions .github/workflows/interop.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,36 +13,35 @@ concurrency:
cancel-in-progress: true

jobs:
run-transport-interop:
name: Run transport interoperability tests
run-interop-tests:
name: Run ${{ matrix.test-type }} Tests
runs-on: ubuntu-22.04
if: success() || failure()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this isn't the purpose of this PR

strategy:
matrix:
test-type:
- folder: transport-interop
action: run-transport-interop-test
- folder: hole-punching-interop
action: run-interop-hole-punch-test

steps:
- name: Free Disk Space (Ubuntu)
# For some reason the original job (libp2p/test-plans) has enough disk space, but this one doesn't.
uses: jlumbroso/[email protected]
with:
tool-cache: true
- name: Checkout Code
uses: actions/checkout@v4

- uses: actions/checkout@v4
- uses: docker/setup-buildx-action@v3
- name: Build image
run: docker buildx build --load -t nim-libp2p-head -f tests/transport-interop/Dockerfile .
- name: Run tests
uses: libp2p/test-plans/.github/actions/run-transport-interop-test@master
with:
test-filter: nim-libp2p-head
extra-versions: ${{ github.workspace }}/tests/transport-interop/version.json
- name: Setup Docker Buildx
uses: docker/setup-buildx-action@v3

run-hole-punching-interop:
name: Run hole-punching interoperability tests
runs-on: ubuntu-22.04
steps:
- uses: actions/checkout@v4
- uses: docker/setup-buildx-action@v3
- name: Build image
run: docker buildx build --load -t nim-libp2p-head -f tests/hole-punching-interop/Dockerfile .
- name: Run tests
uses: libp2p/test-plans/.github/actions/run-interop-hole-punch-test@master
- name: Build Docker Image
run: |
docker buildx build --load -t nim-libp2p-head -f tests/${{ matrix.test-type.folder }}/Dockerfile .

- name: Run Tests
uses: libp2p/test-plans/.github/actions/${{ matrix.test-type.action }}@master
with:
test-filter: nim-libp2p-head
extra-versions: ${{ github.workspace }}/tests/hole-punching-interop/version.json
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 }}
Comment on lines +47 to +50
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be possible to avoid the duplication?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which duplication?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, in both calls, you mean. Not that I know.

Copy link
Contributor

@diegomrsantos diegomrsantos Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }}

Copy link
Collaborator Author

@AlejandroCabeza AlejandroCabeza Sep 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kinda agree with @AlejandroCabeza here. Sometimes, trying to remove duplicates at all cost is detrimental to understanding the code.

Copy link
Contributor

@diegomrsantos diegomrsantos Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll wait for an agreement on this one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, it's not a very strong feeling on my part, we can change if Diego is adamant about it.

Loading