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.

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

Introduce composite action for running ping interop tests #123

Merged
merged 6 commits into from
Feb 1, 2023

Conversation

thomaseizinger
Copy link
Contributor

This PR implements some of the ideas discussed in #111. It is intended to be fully backwards-compatible and can thus be rolled out incrementally across our repositories.

We do a few things here:

  1. GitHub actions needs to already checkout the test-plans repository to access the action's source code. We can reuse this checkout to access the rest of the repository. Thus, if we reference this action via a commit hash, downstream repositories don't suddenly break on changes to the test-runner or other things in here.
  2. Compared to the previous workflow, we don't cache the NPM dependencies. This is a technical limitation originating from (1) as GitHub refuses to cache things outside of $GITHUB_WORKSPACE. This doesn't really matter though as our runner has very few dependencies which are installed in no time.
  3. Because this is now a composite action, we don't need to up/download any artifacts from the source repository. Instead we simply embed this action in a bigger workflow: https://github.com/libp2p/rust-libp2p/blob/test-composite-test-plans-action/.github/workflows/interop-test.yml.
  4. extra-versions should be a space-separated list of JSON files that each contain a JSON object which describes a version to include in the matrix. See the changes to the JSON file in the rust-libp2p PR: https://github.com/libp2p/rust-libp2p/pull/3414/files
  5. The support for extra-version in the test runner is backwards-compatible.
  6. When building local docker images, repositories need to make sure that containerImageID exists locally. As you can see from the rust-libp2p PR, all that is needed is to build and tag the image locally. Because we are within the same workflow, the state of the local docker daemon is preserved.

@@ -12,3 +12,8 @@ jobs:
uses: "./.github/workflows/run-testplans.yml"
with:
dir: "multidim-interop"
run-multidim-interop-ng:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just replace the above?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing the intent was to make sure the existing flow didn't break?

Copy link
Contributor Author

@thomaseizinger thomaseizinger Feb 1, 2023

Choose a reason for hiding this comment

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

Yeah, it is always easy enough to just remove the above later once we are fully migrated.

Copy link
Contributor

@MarcoPolo MarcoPolo left a comment

Choose a reason for hiding this comment

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

This looks good.

After we merge this, I'm guessing the plan is to change {rust,go}-libp2p to use this composite action at the current commit. Then we can rollout the other changes?

@thomaseizinger
Copy link
Contributor Author

This looks good.

After we merge this, I'm guessing the plan is to change {rust,go}-libp2p to use this composite action at the current commit. Then we can rollout the other changes?

Yep, that would be the idea!

@thomaseizinger thomaseizinger merged commit c9130e4 into libp2p:master Feb 1, 2023
@thomaseizinger
Copy link
Contributor Author

After we merge this, I'm guessing the plan is to change {rust,go}-libp2p to use this composite action at the current commit.

I opened a PR for go-libp2p here: libp2p/go-libp2p#2039

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants