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

Return the retrieved checksums on failure #5552

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Leonidas-from-XIV
Copy link
Contributor

Before this change it was not possible to get the actual checksum from the API, because OPAM would download the file, compare checksums, emit an error message and delete the file with the wrong checksum.

So to be able to get the actual checksum, one needed to disable the validation and reimplement the validation on top of the unvalidated API call. This PR returns the invalid checksums.

Before this change it was not possible to get the actual checksum from
the API, because OPAM would download the file, compare checksums, emit
an error message and delete the file with the wrong checksum.

So to be able to get the actual checksum, one needed to disable the
validation and reimplement the validation on top of the unvalidated API
call. This PR returns the invalid checksums.
@hannesm
Copy link
Member

hannesm commented Aug 2, 2023

Out of curiosity, why would you want to keep the retrieved checksum on a failure? Adding various assert false to the codebase doesn't look like a reasonable (easy to maintain & reducing code smell) PR.

@Leonidas-from-XIV
Copy link
Contributor Author

@hannesm If can be useful if you know you're expecting the checksum to change. E.g. you have a checksum specified, then deliberately change the tarball, then whoever is retrieving it using this API can tell you "hey, the checksum changed, is this deliberate? If so, I can actually also update the expected checksum to match".

A bit like the promotion mechanism in Dune, you can of course edit the files manually, but if the change is expected, why not let the computer do it.

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.

None yet

2 participants