-
Notifications
You must be signed in to change notification settings - Fork 276
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
Metadata API: Document serialization "repro" issue #1800
Metadata API: Document serialization "repro" issue #1800
Conversation
Pull Request Test Coverage Report for Build 1805825853
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments look good.
I wonder do we want to add a comment somewhere that even though when deserializing and serializing we don't get byte-for-byte equivalence we do get a consistent signature no matter the whitespaces?
I can see that making the distinction could help... I'll try that (even if the paragraph is a bit long already) |
eb121f6
to
ca9300c
Compare
force pushed:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
tuf/api/metadata.py
Outdated
same because of whitespace issues, even if the signatures are | ||
guaranteed to stay valid. If byte-for-byte equivalence is required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A smart way to mention that signatures are guaranteed to stay valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the clarification, modulo two nits:
- the claim "not guaranteed to be the same" depends on the serializer, which is configurable for both
to_file
andto_bytes
. Maybe it's worth mentioning that? - the reason "because of whitespace issues" is also specific to the default json serializer, and also only part of the problem (e.g. non-guaranteed dict key order is another problem). Maybe it's not necessary to mention a particular reason?
Agreed on both comments, thanks, will update. |
It's not obvious to casual reader that reading metadata and then writing it might not always produce the same file. It's also not immediately obvious why this matters. Document both concepts. Fixes theupdateframework#1392 Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
ca9300c
to
3f3b921
Compare
I solved this by
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update, @jku!
Pull Request Test Coverage Report for Build 1778998638Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
It's not obvious to casual reader that reading metadata and then
writing it might not always produce the same file. It's also not
immediately obvious why this matters.
Document both concepts.
Fixes #1392
Signed-off-by: Jussi Kukkonen jkukkonen@vmware.com
It's a little wordy but I think it may be useful.