Could you please use mock.patch here to ensure proper cleanup? (pseudo code below, untested)

Hi, originally I implemented it as you suggest as it solves this specific issue with minimum other side effects.

However I figured that this way means that if there are any other future changes to the manifest file format, this approach will automatically catch those without a future developer needing to specifically sort their "new" field for example.

Happy to just sort this one key if that is preferable?

I'm not 100% sure of what would be preferable in this case. Your solution is clean and clear, and I quite like it. But it's a little bit more broad than the ticket scope, so part of my role is ensuring that if we go "bigger", we have a good reason.

If we keep the current solution, we should ideally have a test that cover the whole extent of the change. What do you think?

Thanks for your thoughts. I like the current solution because it is really simple, the alternative is this

payload = {
    "paths": dict(sorted(self.hashed_files.items())),  # <-- this line change
    "version": self.manifest_version,
    "hash": self.manifest_hash,
}

I'm less of a fan of this for two reasons. One, its more verbose. Two, what this issue is actually about is the serialisation of the dictionary to a JSON file. We don't really care what order the keys are in the python objects but we do specifically care that the order in the output file is always the same. Therefore I think it makes more sense to do the sorting at the serialisation step aka the json.dumps.

The dictionary itself is constructed directly within the save_manifest() function and both version and hash are strings. Therefore the only part which can change is the order of the keys in paths, so there isn't any other useful tests that can be written to verify the whole extent as nothing else is able to change.

If someone edits the manifest format and introduces a new field then I would expect them to consider whether it is necessary for them to test that this new field is deterministic.

I can add a comment to the save_manifest() function stating that the output file must be deterministic as a reminder to future developers perhaps.

Thank you @matthews-noriker for sharing your reasoning. I've been thinking on what you said and it makes sense, but after a closer inspection, I noticed that the code is already sorting the hashed_files (a few lines above), so I think the best course of action is to:

• store the sorted list of hashed files in a variable
• use it in the two places
• do not use sort_keys in the json dumps call

This fix is a bit more "neutral" while still addressing the reported issue. Not to mention that the double sorting is avoided!

Hi @nessita I have made the changes you have suggested 👍