Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ci: update interop workflow #3331
ci: update interop workflow #3331
Changes from 3 commits
1bead15
d7e84bc
dabd3cd
6e3b873
54a5364
66ab135
23be626
ac94c49
c06369f
fd98709
d1ea824
ac2162c
d640f91
8ce709b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Sometimes we call things test-plans, sometimes we call them interop-tests.
I'd be in favor of calling all of this
interop-tests
.test-plans
is too generic IMO and doesn't tell me anything about what they are testing.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.
Now that we are using plain docker compose, can't we just build the image as part of the workflow here, upload it to GitHub's container registry and reference it here? That seems a lot simpler than storing it as an artifact.
cc @MarcoPolo
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.
In more detail, why don't we have the following:
0.45.0
Once (3) finishes, we can then kick off a dependent workflow that runs a matrix of containers against each other.
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.
You can upload to a container registry if you want. If the containerImageID is a tag, then docker will automatically try to fetch that tag. You don't have to upload image artifacts if you don't want to.
That said, I think the approach of building the container from the commit is better. For one it's guaranteed to be reproducible. You don't have multiple commits from the same PR mutating the tag number. Two, it's the same workflow on master, PRs, or tags. Three, when you introduce a container registry, now you need to deal with authentication, limits, and garbage collecting from that container registry.
The one benefit I see to container registries is them acting as a cache for faster tests. But we can cache layers already using GitHub Actions cache.
I'm not sure the extra work involved in setting up and maintaining a registry is worth it. But of course, if you want to feel free. I'm just sharing my thoughts from thinking about this as well.
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 sharing your thoughts. Perhaps a mix of both would be the best solution?
master
tag would only ever produce one container so no GC needed, i.e. the container for master would be updated every time the master branch gets updated. That hurts reproducibility a bit but it is not a big deal IMO. We already accept that merging one PR affects other PRs.go-libp2p
testing against releases ofrust-libp2p
If we can remove container builds of master and tags from this workflow, then the Dockerfile also gets simpler because we always just build for the current source, no overrides needed.
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.
GitHub makes authenticating and interacting with its own registry really easy so that wouldn't be much work in my experience.
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.
The benefit is faster builds but it also just feels wrong. The source code for a test for e.g. libp2p 0.45 should never change. The version is out there, our users are using it. If we want to take interoperability seriously, I think it is good to constrain ourselves such that changing the test for a particular released version is hard.
Building the binary every time doesn't sit well for me on that front.
It caches per user-defined key which can be completely arbitrary and is not restricted to branches or anything. Bear in mind that each repo only has 10GB here and at least in
rust-libp2p
we already have to jump through hoops to not blow this cache limit for our regular jobs :(It gets reused across all pushes on all open PRs until the next one merges. I myself have usually anywhere from 2-10 PRs open that I work on in parallel i.e. I switch to another PR once I incorporated feedback and wait for the next round of reviews.
My bad, I wrongly assumed that we would still need the "sed"-ing inside the Dockerfile. That should be gone now in any case.
This exact step could have happened when the tag was pushed. It is like pre-built binaries of any other application here on GitHub.
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. I think we're starting to understand each other's points and we agree on the fundamentals.
Just a couple of questions for me:
Maybe there's a misunderstanding somewhere? An open rust-libp2p PR wouldn't do an interop test against rust-libp2p master (we could do that, I'm just not sure it's useful). It runs interop tests against every other released version of libp2p.
I agree with this. I don't think building from source prevents this. If we constrain ourselves to never mutate an image tag, then I think we agree except on the little details. To me a sha256 hash is by definition an immutable tag of an image.
I don't have strong opinions on how rust-libp2p wants to handle building their image. If the project wants to simply submit a container tag that will be used in the interop tests, I only ask that the tag is immutable.
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.
Interesting, I thought we also ran interop tests against our own master branch. @mxinden do I remember this incorrectly?
If not then I agree that it is not useful to have a moving
master
tag.The tag should definitely be immutable. As far as I know, one can append the sha256 hash of a docker image tag to the overall ID and thus guarantee that anyone pulling this image from the registry will get exactly the image with this hash. That is how I would specify those images in test-plans.
Example:
docker.io/library/hello-world@sha256:faa03e786c97f07ef34423fccceeec2398ec8a5759259f94d99078f264e9d7a
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.
If I recall correctly, we did. I was planning to have each pull request tested against
master
with the wo-testground tests, though based on intuition, without specific reasoning. Thinking about it some more, I don't see what kind of bugs this would catch, thus I suggest not investing into PR-to-master testing for now. Thoughts?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.
Another benefit would be for someone to easily test their libp2p-xxx against libp2p-yyy v1.2.3 locally without having checkout a repository, build a Docker image or install libp2p-yyy toolchain, simply pulling and running a pre-build image from a public registry. Just mentioning it here. I am fine with either and fine with Go doing ad-hoc builds and Rust pushing Docker images.
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 think we could save some complexity here if this were just a composite action that we call after building the images rather than a separate job.
That would allow the action within
test-plans
to access local state like built docker images, configuration files without us having to up and download artifacts.@MarcoPolo What was the reasoning for making this a workflow rather than a composite action?
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.
For example, within the same job, we wouldn't need to tar up the container because it would still be accessible by tag.
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.
Good point! I didn't realize I could make a composite action. My knowledge here was out of date. Thanks!
I'll try this out soon. I agree it'll be nice not to have to upload the image. And the upload artifacts are always directories, so it'll be nice to avoid that.
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.
What does
extra-versions
here mean? @MarcoPoloWe seem to be referencing the same image in there that we are also tarring up.
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.
The source of truth for the capabilities of a released libp2p version are defined in
version.ts
. The extra versions is more version data that is added to thatversions.ts
array. This lets you add a "rust-libp2p-head" version to the defined versions and run an interop test between all defined versions on all capabilites. This combined with the ability to filter tests lets you run the interop tests that only include "rust-libp2p-head".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 explaining, this makes sense now.
Is there a usecase where we extend
versions.ts
with more than one image and/or don't filter by the one we just added? It looks like the configuration API of this action could be simpler, i.e. it just needs the contents of one entry inversions.ts
and we always want to filter the tests for this one version.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.
In particular, this seems to be sufficient and perhaps a bit easier to understand:
dir: "multidim-interop"
needs to be specified, can't this be implied by the GitHub action?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.
This should be changed. Originally the "run test-plans" was generic enough that it would work for any
npm test
like thing. But there's little value for this, and it would be nicer to be clearer here.It's true. With the tradeoff of dealing not being able to run this locally as easily. I think it's nice to be able to locally run:
npm test -- --extra-versions=$RUST_LIBP2P/test-plans/version.json --name-filter="rust-libp2p-head"
. Inputs are also only strings, so you'd have to stringify the given example.I agree with all the other points :)
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.
How often do we need that? At the moment, this config file just sits there, not co-located with its consumer and it requires a fair amount of knowledge to figure out what it is for.
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.
Would it make sense to unify the API of the test runner to just accept a series of JSON files with one version each? i.e. it builds the matrix of tests out of all files that are passed to it. Each JSON file would contain exactly one entry.
We could then pass the JSON as a string into the workflow and it writes it to a file. If you really need to run things locally, you just have to quickly copy paste that and make the file.
To me, that seems like an acceptable trade-off for having one less configuration file around per repository :)
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.
Yeah, this seems reasonable 👍 .
The less logic that is in the github actions yaml the better. At least for me, I find this stuff hard to work with since it only runs in CI. The simpler the yaml file is the better.
I don’t see why having this configuration file is bad. I actually think it’s a good thing because:
test-plans/multidim-interop/version.ts
so other implementions test against it.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.
Nice, I'll open an issue on the repo.
You are mentioning some good points. Happy to keep the file, thanks for getting to the bottom of this :)
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.
Any reason for this newline?
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.
Do we still need the protobuf compiler? I thought we fixed this by inlining generated content?
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.
That isn't merged yet :)
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.
For completeness, this is tracked in #3024.
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.
Does this properly depend on all files in src? I thought you needed
wildcard
, but not sure.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.
Yeah good point Marco, it seems to only depend on the directory creation/modification itself. Updated to use
wildcard
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.
🚀 worked like a charm.