Predictable References implementation

[joao-paulo-parity]

Extracted from #327

---

At the moment, the Polkadot PR is expected to fail until the previously-explained lockfile update happens, but then it is expected to work after the lockfile update happens - which, in practice, is an iffy expectation because we're not running the whole Polkadot CI ahead of time.

This whole workflow could be enhanced with the following approaches

Alternative 1: Predictable References

1. Create a Substrate PR which has breaking changes for Polkadot
2. Set up Git ref for the Substrate PR (a "Predictable Reference") such as pr-123

• Note: this is already provided by GitHub as refs/pull/123/head as refs/pull/123/merge

3. Create a companion PR on Polkadot

4. When bot merge is called, the bot will update the Polkadot PR's Substrate reference in Cargo.toml and Cargo.lock via commit, e.g.

   -sp-io = { git = "https://github.com/paritytech/substrate", branch = "master" }
   +sp-io = { git = "https://github.com/paritytech/substrate", rev = "refs/pull/123/merge" }

   By doing the above we expect that the Polkadot PR's CI will pass because it would be already referencing the Substrate PR, which is supposed to solve its breaking changes. Doing so will viabilize checking that both sides are passing before going through with the merges - at the moment it's not viable because the Polkadot PR is expected to be failing at first - therefore we'll overcome "Risk of desynchronization for merge failures" because all CIs should be green before the merge.

5. Wait for all CIs to pass

6. Merge all pull requests simultaneously

For the record: when @joao-paulo-parity explained this to @bkchr on Matrix DMs, the idea of merging the rev = "refs/pull/123/merge" to master was not well received, which motivated the design of Alternative 2.

Alternative 2: Predictable References, but don't commit the updated references

This approach would work similar to Alternative 1 except that we don't end up merging the changes to Cargo.lock and Cargo.toml (related to step 4 of Alternative 1) into master. In this alternative the reference changes of rev = "refs/pull/123/merge" to branch = "master" are not committed to the companion branches, instead they are tested on temporary GitLab branches.

Explained in https://github.com/paritytech/ci_cd/issues/234#issuecomment-1160141699.

[TriplEight]

I finally get the point and it makes sense to me after the meeting lol. Thanks!
So you're coming from an assumption that https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-git-repositories (with rev = "refs/pull/493/head" and GitHub in particular exposes a reference to the most recent commit of every pull request as shown) will not work for the contributors' PRs from the forked repos?

[joao-paulo-parity]

So you're coming from an assumption that https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-git-repositories (with rev = "refs/pull/493/head" and GitHub in particular exposes a reference to the most recent commit of every pull request as shown) will not work for the contributors' PRs from the forked repos?

I thought refs/pull/*/head would not work, but if it does, that's potentially less work to be done. I don't know if it's advised to use that though since it's not something we have control over: the ref is owned by GitHub. For instance, if it would be necessary to move the repository to another platform at some point, those refs would be permanently gone; I don't think this should matter for releases since I infer such refs would be cleaned away before we do a release, but it could have an effect on external consumers. Something else to consider is that using refs/pull/*/head might be more difficult to understand or somehow off-putting for consumers.

refs/pull/*/merge (master + PR) is worth mentioning and maybe suits our use-case better. The caveat is that GitHub doesn't update it when master changes, but I still think it makes more sense to use that rather than refs/pull/*/head anyways.

Then we need to start thinking about how this "Predictable references" idea would interact with with pre-merge pipelines, which to be very clear, as far as I'm concerned, definitely doesn't have to be delivered together with this.

I think the "pre-merge pipelines for pull requests" GitLab feature would work well in conjunction to Alternative 1 because it doesn't incur the creation of new SHAs (which would have to be updated in all the companions). Alternatively, we could have a branch which would be updated via Git push when master changes (i.e. would be used instead of refs/pull/*/merge since it doesn't update automatically and we can't edit it) but it has a big downside that we'd need to update to this new SHA in all the companions, which I find less practical.

[TriplEight]

since it's not something we have control over

everyone git checkout's to branches/PRs HEADs so it's reliable much.

if it would be necessary to move the repository to another platform at some point, those refs would be permanently gone

we'll have a whole bunch of higher-level migrations from GitHub API in such a case, so it won't be the main problem.

such refs would be cleaned away before we do a release

yup, automatically by GitHub, so we shouldn't care. But it's important to keep them within the "Companion PR" job only, so no chances it gets released.

refs/pull/*/merge

This is a great point in the view of the upcoming pre-merge pipelines! The idea with them that they will be launched after the PR is marked as ready. This makes a good place to execute Companion jobs too. So I'd give it a go unless there's some corner cases that would oppose this logic.

pre-merge pipelines ... definitely doesn't have to be delivered together with this

yeah, for sure. But as they will be (or will have to be) implemented separately, I see no bad in starting here. With a due explanation of course.

... but it has a big downside that we'd need to update to this new SHA in all the companions, which I find less practical.

I think in our case it makes sense to just do a rebase within a Companion job if GitHub doesn't update refs/pull/*/merge

[joao-paulo-parity]

For the record I've added the feedback I got from @bkchr on Matrix about the idea of merging the rev = "refs/pull/123/merge" to master.

[joao-paulo-parity]

Added the needs planning label because I think it would be valuable for the implementer to more thoroughly gather feedback on the alternatives from stakeholders and also consider different solutions.

[mordamax]

blocked until we have the decision on monorepo