-
Notifications
You must be signed in to change notification settings - Fork 2
Update Consistent Snapshots section #6
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
Conversation
I barely touched the "Producing Consistent Snapshots" and "Snapshot Process" sections. But I think a revision of those two is due. What do others think? @JustinCappos, @mnm678? |
consistent snapshots used to require a hash digest prefix in filenames for metadata and target files, now only target files add a hash digest prefix and metadata uses a version number.
f714308
to
5d2cadd
Compare
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 left a couple of comments, but overall this looks good. I agree that a revision of "Producing Consistent Snapshots" and "Snapshot Process" would be a good idea.
in its filename: | ||
|
||
HASH.FILENAME | ||
where HASH is the `hex digest`__ of the `SHA-256`__ hash of the file |
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.
Do we only allow for sha-256 hashes?
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.
This was already in the pep, I just moved it up here. Should we leave the choice of algo open? It should be an algorithm that is also used when creating targets hashes, to be added in the (delegated) targets file. Because that's how the client builds the filename, right? (cc @JustinCappos, @trishankatdatadog).
Btw. the spec does not prescribe an algorithm. It does, however, point to an outdated version of this pep. We should remove those pointers in the spec.
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.
We can choose the secure hash algorithm here. SHA-256 is fine.
VERSION_NUMBER.ROLENAME.json, | ||
where VERSION_NUMBER is an incrementing integer, and ROLENAME is one of the | ||
top-level metadata roles -- *root*, *snapshot* or *targets* -- or one of | ||
the delegated targets roles -- *bins* or *bin <i>*. |
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.
Elsewhere, I think we refer to the delegated bins roles as bin-n. I don't have a particular preference, but I think we should be consistent.
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 agree. I think it's not fully consistent elsewhere either. We should, probably in a separate PR, go through the entire document and clearly disambiguate between bins and bin-n roles/metadata.
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.
Looks good overall. We can list SHA-256 by name in the PEP
Thanks for the reviews, @JustinCappos and @mnm678! |
consistent snapshots used to require a hash digest prefix in filenames for metadata and target files, now only target files add a hash digest prefix and metadata uses a version number.