-
Notifications
You must be signed in to change notification settings - Fork 272
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
TUF Python Client Example/Tutorial #1675
TUF Python Client Example/Tutorial #1675
Conversation
I am still tagging it as WIP to improve in general. The |
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.
nice, thanks. Some comments (some of these could be left for future work):
- I think argparse is probably a good choice: it's verbose but part of the base library and will allow multiple subcommands without making a mess
- init should be implicit, I think (as in it should always happen)? it's the correct example for real clients and as long as the printed output is valid it should never be a problem either (also if init is implicit, then it can probably be combined with tuf_updater)
- I would not use "[INFO]" and "[ERROR]" like this: to me it implies you are using logging and that I can decrease/increase verbosity but I can't, it's just print()... maybe you should be using logging (and set default level to INFO or something)?
- linting: we want this code linted (with the same config that ngclient uses)
- docs: I think in the end the documentation should get included in the published docs (probably under a new top level "examples" title)... this does not have to happen right now but I think we should delay reviewing and polishing the doc until we see the published form. My gut feeling is that we should not include details like how to clone the repo or where to find the example code etc: these are things that change over time and then someone would need to update them. We have a "instructions for contributors" document, let's link to that if we really have to but let's not repeat ourselves.
Pull Request Test Coverage Report for Build 1536678421
💛 - 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.
Cool stuff! I suggest we start running pylint/black/isort over code in the new examples directory too. Can you add the directory to the lint
directive in tox.ini
?
Also, I think the printed info text could be slightly improved (I say this as a non-native English speaker). :)
Maybe we can ask our technical writer and chief of words @jhdalek55 for help with that. |
Yes, there are more fancy libraries, but I think argparse will do the work without extra dependencies.
Yes, indeed. I was in doubt about which direction to choose.
Actually, I started with logging, but I tried to make it less complex in the sense most of the basic tutorials try to use "print".
👍
I understand that we do not want to repeat ourselves, making it more maintainable, and I agree. |
@jhdalek55 ...I'm happy to work on any English language issues. Just let me know what needs to be reviewed and when. I generally prefer to hold off on reviews until the technical content is more or less confirmed. |
Same, either way works for me. On the question of "is this good for a tutorial/example?", I think the goals should be
in this order: my opinion is that good example code is more important than a tutorial that literally anyone could follow. |
The last commit addresses more general aspects such as lint, README format, etc. |
Some questions:
|
Let me finalize my example in that PR and gather some more feedback. I'm still unsure, if a markdown wouldn't be more appropriate, in terms of making the accompanying text more readable. IMO the question boils down to readability vs. ease of testing. (see #808 (comment) and in-toto/demo#7 for related discussion about markdown doc testing) |
I think your code and 1685 are different for two reasons:
So I don't think there's a need for these two solutions to look similar.
For the client app my thinking has been that it should need to handle RepositoryError only (although I see you had to handle some requests issues as well -- we can consider those after this merges): RepositoryError means the update cannot continue because of an issue with TUF metadata: there could be many specific reasons but the client code cannot possibly deal with any of them. |
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.
wow, ended up with a lot of comments -- if this is still WIP don't mind me
path = updater.find_cached_target(info) | ||
print(f"Cached target {target} verified") | ||
if path: | ||
print(f"Target is already available in {DOWNLOAD_DIR}/{info.path}") |
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 is not always correct -- info.path is the url path which may or may not match the filename on disk. path
contains the filesystem path
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 will review what I did, but insights are welcome.
This should probably be a future PR: we want the example tested by CI (should file a new issue for that) For inspiration, I have some (quite bad) tests for a CLI app in https://github.com/vmware-labs/repository-editor-for-tuf/blob/main/tests/test_cli.py -- I think just a very simple test is fine: run a few commands, check if output contains correct keywords, check that files exist where they should |
I am still looking if |
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 think the overall structure is good: client_args construction could be in its own function but so far it's so simple that I don't mind the current way.
Can you enable mypy for this? (currently still happens in setup.cfg I 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.
I think this looks good: we'll definitely want to keep improving but this looks like a fine first step. The commit history could be squashed as it's a little random but since it's new code I don't really mind either way.
Anyone else want to have opinions here (@joshuagl, @sechkova)?
Potential future work:
- Consider if we want the README published on RTD, modify it as required if we do
- Add a simple test for the example
- support other server locations than hard-coded localhost:8000
- support boostrapping root.json for other servers (like the repository generated by the repository example) somehow
- document the application decisions better (as an example "why is is local metadata stored in $HOME/.local/share/", "why not download first root.json over http?"). I didn't do this in the review as I think it's a tricky balance: too much detail means no-one will read it...
- start thinking about how to use examples to show that TUF actually works, and prevents attacks. This might be more related to repository examples but ...
I didn't actually run this before mysefl but now I did: I seem to get debug level output if i don't provide a verbosity level? This clearly seems like a bug? |
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)
It is a simple example of TUF ngclient implementation. This example contains a README.rst that is a tutorial/how-to-use this simple client using static test data from TUF repository. The code aims to be straightforward implementation, using basic concepts from Python and Command Line Interface. This is part of theupdateframework#1518 Signed-off-by: Kairo de Araujo <[email protected]>
- Added the lint to examples - README format moved from Restructuredtext to Markdown - Removed the [INFO] and [ERROR] from output, to avoid confundint with logging structure Signed-off-by: Kairo de Araujo <[email protected]>
Using ``print`` for high-level client output Option to see ``tuf.ngclient`` logging output Update the README. Signed-off-by: Kairo de Araujo <[email protected]>
To highlight TUF ngclient exceptions. Signed-off-by: Kairo de Araujo <[email protected]>
This commit adds many comments from the WIP stage Signed-off-by: Kairo de Araujo <[email protected]>
To simple exemplify, the updater calls are added to a single block that handles the exceptions Signed-off-by: Kairo de Araujo <[email protected]>
- Simplify README and better keywords - Fix the verbosity - Better docstrings - Client flow for init and main are clear Signed-off-by: Kairo de Araujo <[email protected]>
without -v is ERROR logging, -v WARNINGS logging, -vv INFO logging and -vvv (+) DEBUG logging. Signed-off-by: Kairo de Araujo <[email protected]>
5082b16
to
1344410
Compare
Merging this. Please file a new issue for adding a test for this example. Maybe a new issue for the RTD documention inclusion as well? I'm not 100% sure we should include the example there but maybe someone should take a stab and see if it works well and whether it needs any changes. |
It is a simple example of TUF ngclient implementation.
This example contains a README.rst that is a tutorial/how-to-use
this simple client using static test data from TUF repository.
The code aims to be straightforward implementation, using basic
concepts from Python and Command Line Interface.
This is part of #1518
Signed-off-by: Kairo de Araujo [email protected]
Please fill in the fields below to submit a pull request. The more information
that is provided, the better.
Description of the changes being introduced by the pull request:
Please verify and check that the pull request fulfills the following
requirements: