Update updater workflow doc in client/updater.py #1172
Conversation
There was a problem hiding this comment.
Thanks Martin, I appreciate you taking the time to ensure documentation is up-to-date.
The previous version, while inaccurate, was a bit more descriptive than what you've included here. I don't think we want to embed the full detailed client workflow, but a bit more description of "when necessary" might help. What do you think?
|
I will see what I can do. |
Signed-off-by: Martin Vrachev <[email protected]>
Signed-off-by: Martin Vrachev <[email protected]>
MVrachev
force-pushed
the
update-updater-doc
branch
from
5a85eb2 to
138be5d
Compare
There was a problem hiding this comment.
Thanks for taking a stab at this, @MVrachev, it is highly appreciated! I just cross-checked with #808 and there is probably more work needed on tuf/client/README.md, but updating the workflow steps is definitely a good start (I mentioned your work in #808 (comment)).
Also, I haven't cross-checked your updates with the actual implementation, maybe either @jku or @sechkova, who are more familiar with the client, could also take a look?
In addition to my inline comments here are some high-level observations:
- There's a bit of a mix between indicative ("loads the trusted root metadata") and imperative ("Verify the desired target") mood here, in one case even within a sentence ("Download and validates the timestamp metadata"). Please use one consistently.
- There's also a mix of including ("Tuf loads the trusted root metadata") and omitting ("Updates the root metadata") the sentence subject. I think this can be made consistent too.
- Meta: Please try to separate your commits in a meaningful way and add a little bit more info to the commit message (for instance it would have been interesting to know whether you updated this based on the implementation or on the spec or both. EDIT: I just saw that you do mention this in the PR description. 👍 ).
Thanks!
|
|
||
| 2. TUF downloads and verifies timestamp.json. | ||
| 1. Tuf loads the trusted root metadata file.s |
There was a problem hiding this comment.
| 1. Tuf loads the trusted root metadata file.s | |
| 1. TUF loads the trusted root metadata file. |
| 3. If timestamp.json indicates that snapshot.json has changed, TUF downloads and | ||
| verifies snapshot.json. | ||
| 2. Updates the root metadata file to the latest trustworthy version. | ||
| Establish a trusted line of continuity to the latest root version. |
There was a problem hiding this comment.
Can you put those two sentences somehow into relation to each other?
| file. | ||
| 5. If snapshot.json indicates that any of the top-level targets metadata and/or | ||
| delegated targets metadata has changed, TUF downloads and verifies targets.json | ||
| and/or all changed delegated target files. |
There was a problem hiding this comment.
Note that there is a difference between target (w/o s) and targets (with s) files. It's easy to mix them up, that's why I usually say "targets metadata files" to better distinguish from non-metadata target files.
| and/or all changed delegated target files. | |
| and/or all changed delegated targets metadata files. |
There was a problem hiding this comment.
Also, is this even accurate? Shouldn't the client, according to the spec, only download delegated targets metadata that is relevant for the desired target file, instead of all changed?
There was a problem hiding this comment.
Also, I think we should mention the traversal of the delegation graph here in order to resolve the correct targets metadata for a target file.
|
|
||
| 7. TUF downloads and verifies the file and then makes the file available to | ||
| the software update system. | ||
| 6. Verify the desired target against its target's metadata. |
There was a problem hiding this comment.
How does it verify the target before downloading it?
| 7. The software update system instructs TUF to download a specific target | ||
| file. | ||
|
|
||
| 8. TUF downloads and verifies the file and then makes the file available to |
There was a problem hiding this comment.
What does "verifies the file" mean here?
| @@ -29,24 +29,28 @@ | |||
|
|
|||
| An overview of the update process: | |||
|
|
|||
| 1. The software update system instructs TUF to check for updates. | |||
| 0. The software update system instructs TUF to check for updates. | |||
There was a problem hiding this comment.
All comments in README.md above apply to tuf/client/updater.py as well. Maybe we should de-duplicate? E.g. just point to the README.md here? What do others think?
|
Converted to draft while there are comments left to be addressed, so that I don't keep re-reading the discussion to see what the status is. |
|
Will update the pr soon as I find the time. |
No problem, whenever you can. |
|
Closing as this is no longer relevant as we are going to remove this code soon. |
Fixes issue #: None
Description of the changes being introduced by the pull request:
Update updater workflow doc in
tuf/client/updater.py.The current workflow, described in this doc, is outdated for more than a year.
I have used the specification workflow in point 5 from the specification.
Signed-off-by: Martin Vrachev [email protected]
Please verify and check that the pull request fulfills the following
requirements: