-
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
doc: basic hash bin delegation repo example + test #1700
doc: basic hash bin delegation repo example + test #1700
Conversation
Pull Request Test Coverage Report for Build 1525217612Warning: 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 |
Add 1.0.0 announcment document and point to it in main README. TODO: - Commit message - PR (blocks on theupdateframework#1693, theupdateframework#1675, maybe theupdateframework#1700)
Signed-off-by: Lukas Puehringer <[email protected]>
As 'repository_tool' and 'repository_lib' are being deprecated, hash bin delegation interfaces are no longer available in this implementation. The example code in this file demonstrates how to easily implement those interfaces, and how to use them together with the TUF metadata API, to perform hash bin delegation. Note, the hash bin delegation logic in this example is largely copied from repository_{lib, tool}, and modernized and simplified for this purpose. Signed-off-by: Lukas Puehringer <[email protected]>
Signed-off-by: Lukas Puehringer <[email protected]>
1b90ab3
to
b8cf1c0
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.
LGTM, I don't have any specific comments on this.
In the future this definitely looks like something that would make sense to package with the repository API somehow:
- manage a "bin container" Targets automatically so repository API user can just "add a new target to bin container" (and in reality target is added to the correct bin)
- figure out how to handle NUMBER_OF_BINS changes (at least provide advice on when and how to handle growth in targets numbers) and other maintenance things.
... but like a lot of repository functionality it looks like something I'd really rather see experimented in an implementation before we add it to any library code
I had the same thought, while revising (and trying to re-understand) the binning logic. In reality a user does not care about the exact NUMBER_OF_BINS, but rather about an "appropriate" distribution of target files over bins. So it probably makes more sense to provide an interface, where the user specifies the expected number of target files (or corresponding classes of target numbers). |
# The available digits in the hexadecimal representation of the number of bins | ||
# (minus one, counting starts at zero) determines the length of any hash prefix, | ||
# i.e. how many left digits need to be considered to assign the hash to a bin. | ||
PREFIX_LEN = len(f"{NUMBER_OF_BINS - 1:x}") # 2 |
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.
Maybe add a simple word or comment on what 1:x
does.
I had to find it online and because I hadn't used it, it wasn't trivial to me.
In that context, it could be useful to explain somewhere why we use hexadecimal strings.
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 comment, @MVrachev! I just pushed a commit trying to make this clearer. Let me know what you think.
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.
Only one small suggestion.
Other than that as a reader who has never read anything specific about hash bin delegations
it seems understandable.
LGTM!
# by each individual bin (BIN_SIZE): | ||
# | ||
# The prefix length is the number of digits in the hexadecimal representation | ||
# (see ':x') of the number of bins minus one (counting starts at zero), i.e. ... |
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.
# (see ':x') of the number of bins minus one (counting starts at zero), i.e. ... | |
# (see "x" integer representation from https://docs.python.org/3/library/string.html#module-string) | |
of the number of bins minus one (counting starts at zero), i.e. ... |
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, Martin! I included a slightly modified version of this suggestion. Merging when CI/CD returns...
Tries to clarify the introductory text in the hash bin delegation example. Signed-off-by: Lukas Puehringer <[email protected]>
11bfb08
to
ef388da
Compare
Following parallel merges of theupdateframework#1700 (added new test method), and theupdateframework#1710 (started running mypy on tests), ci/cd fails in the develop branch. This is fixed in this patch. Signed-off-by: Lukas Puehringer <[email protected]>
Fixes #1673 (together with #1685)
Description of the changes being introduced by the pull request:
As 'repository_tool' and 'repository_lib' are being deprecated, hash bin delegation interfaces are no longer available in this implementation. The example code in this file demonstrates how to easily implement those interfaces, and how to use them together with the TUF metadata API, to perform hash bin delegation.
Note, the hash bin delegation logic in this example is largely copied from 'repository_{lib, tool}', but modernized and simplified for this purpose.
Notes to reviewers
Thanks!!
Please verify and check that the pull request fulfills the following
requirements: