Rethink the manifest checksum check

[achilleas-k]

The manifest checksum check has become a bit annoying. It's not required, so a failing check doesn't block a PR, but it very often creates merge conflicts, requiring extra work to rebase PRs that should would otherwise allow automatic rebasing. I have a couple of ideas for how to get around these issues while also keeping the information they provide.

Background

Skip this section if you know what the manifest checksums are and their purpose.

The checksums and the CI test were introduced in #1380. For anyone who's not familiar with the need for the checks, but also as a reminder for everyone else, the purpose of the checksums is to signal to patch authors and reviewers that a certain commit has an effect on one or more generated manifests. This can be very useful in two scenarios:

1. A contributor is authoring a commit that refactors some part of manifest generation to make the code easier to read, but should be functionally identical. The commit or PR should have no changes in the checksum file, because manifests should not be affected by the change. The CI test verifies that the checksums are valid.
2. A contributor is authoring a commit that changes specific image types, or test configurations. Each commit that affects a manifest should be accompanied by a change in the checksums file and should be limited to just the configurations it has affected, i.e. there were no unintended side effects that changed unrelated images or configs.

In the past, we used to have full manifests checked into the repository for this same purpose. Commits that changed manifests showed the exact change in the generated manifest itself for reviewing and tracing. However, the people who remember those times do not have fond memories of it. I (I think it was me) proposed checksums as a way to have a signal that something changed without needing big manifests checked into the repository and also big, hard to read patches that eventually get ignored.

Ideas for replacement

1. Replace manifest checksums file and check with a CI check and GitHub action

I think with a bit of engineering we can automate the notification that manifests have changed. This is also something we tried in the past but got rid of because it was a bit useless. The old solution was simply a comment on the PR by Schutzbot that mentioned that manifests changed, without any extra information. I think a solution that might work is something that integrates with GitHub's annotations, or a comment that lists every commit in a PR that affects a manifest along with a link to a diff of the actual manifest (or a folded codeblock). This can be done in a way that blocks the PR and forces reviewing, so reviewers are encouraged to check each diff and dismiss it if it's valid.

Benefits:

• Requires no interaction by authors and no knowledge for external contributors.
• Enforces reviewing of changes in manifests with more information than the current system.

Drawbacks:

• Can get cumbersome to check large changes, especially if presented as diffs of json. It's still, in the worst case, as good as the current solution though in terms of information provided (i.e. "this commit changed these images"). The json diff can be mitigated by converting to yaml instead (more diffable).
• Will require some engineering.

2. Split the manifest checksums file into multiple files, one per line

This idea addresses the merge conflict issue directly, but doesn't make it go away completely. It simply gets rid of the issue where some manifest changes in main and a PR may create merge/rebase conflicts in the file but could be avoided if they are in different manifests. This doesn't get rid of the issue where two PRs conflict based on changes in the same file, which does happen. But it would solve the problem where a PR makes changes in separate manifests from the ones changed in main (see the current state of #1565, for example, which introduces new images and the lines can't be cleanly merged into the modified file in main).

Benefits:

• Very little engineering required.

Drawbacks:

• Can create inconsistent states in main (which is also possible with the current approach). For example, if a PR gets merged that changes all rpm repo snapshot URLs, it affects all manifests. Then if another PR introduces new images, with new files for the checksums, they will be created with the old repo snapshots. The PR can be merged cleanly, but the new manifests will have checksums computed with the old snapshots, so essentially some checksums in main are invalid and will require a change in any PR that gets created afterwards (which can be confusing since the change comes from a different PR).

[ondrejbudai]

What about a rebase bot? When triggered (there are multiple options), it would rebase the PR, fix the checksums, push into the branch, and if the PR was previously approved, it would re-approve it.

[achilleas-k]

What about a rebase bot? When triggered (there are multiple options), it would rebase the PR, fix the checksums, push into the branch, and if the PR was previously approved, it would re-approve it.

That seems like a bit more work and slightly less useful than having a bot that gives you diffs for each commit. The other one would be useful to run locally as well.

[bcl]

I like #2, despite the possible drawbacks. snapshot updates are less frequent than merge conflicts :) It's also important that I be able to run it locally. I don't really like the idea of a rebase bot fiddling with the PR, potentially causing problems if I have to update it to fix something else, and not being able to see errors before I create or update the PR.

[achilleas-k]

It's also important that I be able to run it locally.

This is an important requirement for me too.

I'd like to see if I can get the first solution working simply before dismissing it outright. But if I do, it'll be something that works locally first.

[achilleas-k]

We're giving the second solution a shot: #1625

[achilleas-k]

The check is now marked as required for PRs.

[lzap]

#1878

[lzap]

Another proposal of mine: #2375

[lzap]

I filed somewhat related PR: #2382

What really bugs me today is the slow GitHub UI. And I was wondering why we switched to individual files in the first place. The way I see manifests is: conflicts are a feature, not a bug.

My workflow today is if I encounter a conflict, I do not even think about it and just do git rebase ... -X ours -exec. We can even make this little bit easier by telling git to add "ours" automatically via .gitattributes configuration:

test/data/manifest-checksums merge=ours

On top of that, we can create a helper script that will do all the rebase -exec magic which I usually dig in my shell history. This does not bugs me, slow GitHub UI does.

---

Crazy (Bad) Idea: Sum part of the filename

What instead of storing the sum in the contents of the files we would do it as part of the file. If we used not sha256 but crc64 encoded in base32 encoding, it would look something like this:

rhel_9.9-x86_64-ec2_sap-empty-J5YQ2P6A7VBN3
rhel_9.9-x86_64-edge_ami-edge_ostree_pull_user-7YTDCM2L4WQRS
rhel_9.9-x86_64-edge_ami-edge_ostree_pull_user_minimalenv-B4H7KZX3NM5P2
rhel_9.9-x86_64-edge_commit-edge_ostree_pull_user-R2W6FQA4GTYV5
rhel_9.9-x86_64-edge_commit-embed_containers-M7C3L2XVRKJ5H
rhel_9.9-x86_64-edge_commit-ostree-P5Z4N7JCHW2B3

Since the files are empty, it could make GitHub UI faster, this is yet to be confirmed but if I am not mistaken, git conflicts are non issue for any update, addition or deletion. I did analysis of the whole history of the checksums and the most offending were most likely updates to the "all_cutomization" checksums. But with this approach, update is always:

• Do git rm of a file.
• Do git add of a uniquely named file.

There will be no conflicts, delete and delete is not a one. However what is worse is that regeneration of manifest was always a changed file so it was quick and easy git commit -a, with this, it is gonna be git add -A; git commit. Of course, rebasing without regenerating checksums will leave some "extra" files which are harmful, but any next regeneration will clean them up. This cleanups could be distracting and irrelevant tho, we would need to enforce those extra files do not exist on CICD.

And this is the point where I finally realized - conflicts are the feature, not bug.