libp2p/test-plans

feat: have Makefile accept branches and tags in addition to commit hash

p-shahi opened this issue · 8 comments

          As an aside: it would be nice if the setup didn’t require us to insert a hash here, but accepted everything that git itself accepts as a reference (branches, tags, etc.).

Originally posted by @marten-seemann in #192 (review)

Copying context from that PR:

We can do something like this, but we should pin some hash. Branch names and tags are mutable.

@MarcoPolo


Since we don't retract / change tags, they're not really mutable.

And even if we did, that would be a feature, not a bug. We're not creating a system here that's resistant against supply chain attacks (that's the reason we use hashes in Unified CI), but a system to test the releases that we create ourselves. Of course if we say that we want to test vX, this should test vX.
Addressing them by their tag name makes the configuration a lot more readable than a hash.

@marten-seemann

I'm not against this. I only care that these tests are as reproducible as possible. Using only a mutable pointer, even if we in practice don't mutate it, is not good enough.

I think a solution could be that we use both a tag/branch (mutable pointer) and an expected commit hash (or tarball hash). Builds will fail if pulling from that tag/branch does not lead to the expected hash. This makes the configuration readable and holds our reproducibility goal.

Note that I'm not concerned with supply chain attacks here. The goal is to be able to reproduce a test environment from a given commit of test-plans.


Implementation guide:

  1. Add a helper that will fetch a tarball from github with a given tag/branch and verify it matches an expected hash. This will probably be the sha256 of the tarball. If the expected hash is wrong it should fail the CI and print a helpful message on what went wrong.
  2. Add a helper that can tell you what the expected hash should be for a given tag/branch so that you could feed it into 1.

These helpers should be implemented in TypeScript.

Version tags are what version managers use to include dependencies, so this is the one thing that we should be testing. Using a commit hash is fundamentally the wrong abstraction layer.

I'd argue that if there was an implementation that did mutate their version tag (and again, you really shouldn't do that), AND if that caused a regression, having the interop tests catch that would be a quite valuable feature, not a bug. Users including that version would get the version described by the tag, not the commit hash.

Version managers pin the hashes of their dependencies (e.g. go.sum, package.lock, Cargo.lock, etc.)

At the time the project is created / the dependency is updated. Not sure why this is an argument for making our lives difficult by specifying a commit hash instead of a tag.

The solution I'm suggesting is exactly what package managers do. You specify a tag, and it keeps track of the expected hash for that tag.

With the difference that package managers do it automatically, while for us it would be an additional manual step. I still don't understand what we're defending against that justifies making our release process less smooth than it could be.

This could be automatic. You could point to a tag/branch and run make lock and that generates a lock file that will check.

I still don't understand what we're defending against that justifies making our release process less smooth than it could be.

This is about making the build process reproducible. For the exact same reasons you want to commit lock files into your repo when building an app. The interop tester is an app that tests if things can talk to each other.