Ngclient api polish #1604

jku · 2021-10-05T09:04:17Z

NOTE: This PR including the description has been updated based on initial comments: this means the first comments look a bit strange now...

This is part 2 of #1580 (ngclient API tweaks): originally I thought we could just replicate old client API but the more I've used it, the more I've wanted to change it. So here's my proposal.

The PR contains a couple of semi-related changes:

shorten get_one_valid_targetinfo() to get_targetinfo()
Replace the unintuitive updated_targets() with find_cached_target() which operates almost exactly like download_target() but looks at local disk only.
Add a target_dir constructor argument that both find_cached_target() and download_target() can use: this lets Updater choose the file name. Both methods also take an optional filepath so clients can control the filename & path.

So the two ways of using the API are:

with direct local filepaths (when client knows the local filepath it wants to use)

updater = Updater(...)
info = updater.get_targetinfo("my/target")
if not updater.find_cached_target(info, path)
    updater.download_target(info, path)

with cache directory (Updater decides filename)

updater = Updater(..., target_dir="cache/dir/")
info = updater.get_targetinfo("my/target")
path = updater.find_cached_target(info)
if path is None:
    path = updater.download_target(info)
# path == "cache/dir/my%2Ftarget"

Based on how much better the test code now looks, I think we're approaching the correct design here...

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

joshuagl · 2021-10-05T10:51:23Z

This is draft as I'm still thinking if I should add a new optional constructor argument target_dir to Updater:

I think this would be a good change. It's good to have the API versatility to download targets to different locations, but I think downloading all targets to the same location is a common enough pattern that this optional constructor argument makes sense.

sechkova · 2021-10-05T11:55:44Z

This is draft as I'm still thinking if I should add a new optional constructor argument target_dir to Updater:

I think this would be a good change. It's good to have the API versatility to download targets to different locations, but I think downloading all targets to the same location is a common enough pattern that this optional constructor argument makes sense.

It sounds good but does the API need to be that flexible, giving the option to set the target_dir at two places?
I am trying to imagine when the directory argument of download_target could be needed then:

A client with more than one cache dir wants to call download_target on the same updater but with a different cache directory?
The cache dir is not known yet by the client when constructing the Updater but only later during the download_target call?

jku · 2021-10-05T12:13:40Z

It sounds good but does the API needs to be that flexible, giving the option to set the target_dir at two places? I am trying to imagine when the directory argument of download_target could be needed then:

Good point! If we could drop the directory argument completely from the actual find_cached_target()/download_target() calls that would be nice. Then there would be two ways to use the API:

With cache dir

updater = Updater(..., target_dir="my/cache/dir/")
info = updater.get_targetinfo("my/target")
path = updater.find_cached_target(info)
if path is None:
    path = updater.download_target(info)

With explicit filepaths:

updater = Updater(...)
info = updater.get_targetinfo("my/target")
if not updater.find_cached_target(info, "/my/local/target")
    updater.download_target(info, "/my/local/target")

(I'm just assuming refresh() is not required in these examples)

A client with more than one cache dir wants to call download_target on the same updater but with a different cache directory?

The cache dir is not known yet by the client when constructing the Updater but only later during the download_target call?

I think nothing prevents target_dir from being a modifiable public attribute that client could change if need be

sechkova · 2021-10-05T12:22:19Z

Good point! If we could drop the directory argument completely from the actual find_cached_target()/download_target() calls that would be nice. Then there would be two ways to use the API:

And we can escape the mutually exclusive directory<->filepath parameters! I think I'm convinced.

coveralls · 2021-10-05T13:28:07Z

Pull Request Test Coverage Report for Build 1367591861

Warning: 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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

14 of 14 (100.0%) changed or added relevant lines in 1 file are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage increased (+1.0%) to 98.352%

Files with Coverage Reduction	New Missed Lines	%
tuf/ngclient/updater.py	6	94.44%

Totals
Change from base Build 1363818148:	1.0%
Covered Lines:	3706
Relevant Lines:	3734

💛 - Coveralls

jku · 2021-10-06T08:59:19Z

I have incorporated the suggestions and am quite happy with the results (the PR message is also updated so check the current status there): The test code looks a lot more readable so we may be on the right track here. The only thing I don't particularly like is the constructor with 4 or even five arguments but it seems a like a good compromise

I believe the PR is ready for real review.

This will definitely conflict with #1591 : I would expect that one to merge first so I'll clean up afterwards

sechkova

I have no objections here.

tuf/ngclient/updater.py

MVrachev

Overall, I like the changes.
I have only two small comments.

tuf/ngclient/updater.py

MVrachev · 2021-10-07T10:03:56Z

tuf/ngclient/updater.py

+        If the ``target_dir`` argument was given to constructor, ``filepath``
+        argument is not required here: in this case a filename will be
+        generated based on the target path like in ``download_target()``


What will happen if target_dir is set and filepath is given?
From the code, I see that filepath is with higher priority and will be used.
Do we need to document that?

makes sense to document what the arguments default value does at least... I think that (and the text you quote) should cover all the cases?

I'd remove the "like in download_target()" part. The reader of this docstring might not (and does not need to) know about download_target when reading this.

Also, I think the text could be misinterpreted that a previously set target_dir has precedence over a passed filepath, or that they are somehow used together.

Maybe a slightly more detailed filepath arg description would eliminate the need for this paragraph here? See below for a suggestion.

jku · 2021-10-08T06:39:30Z

Updated docstrings and comments based on feedback (only "ngclient: Simplify caching" (d717775) has changes)

MVrachev

LGTM!

sechkova · 2021-10-12T09:56:11Z

This will definitely conflict with #1591 : I would expect that one to merge first so I'll clean up afterwards

There it is, #1591 is merged, could you fix the conflicts in this one?

jku · 2021-10-21T11:02:52Z

CI failure handled in #1628

jku · 2021-10-21T12:28:36Z

rebased to current develop to get tests passing again

lukpueh

Amazing work, @jku et al.! I'm just catching up on the new client and I like it lot. Here are a few comments from reading through ngclient/updater.py line by line.

(Haven't looked at the tests yet)

tuf/ngclient/updater.py

lukpueh · 2021-10-26T12:15:56Z

tuf/ngclient/updater.py

+        If the ``target_dir`` argument was given to constructor, ``filepath``
+        argument is not required here: in this case a filename will be
+        generated based on the target path like in ``download_target()``


I'd remove the "like in download_target()" part. The reader of this docstring might not (and does not need to) know about download_target when reading this.

lukpueh · 2021-10-26T12:26:47Z

tuf/ngclient/updater.py

+        If the ``target_dir`` argument was given to constructor, ``filepath``
+        argument is not required here: in this case a filename will be
+        generated based on the target path like in ``download_target()``


Also, I think the text could be misinterpreted that a previously set target_dir has precedence over a passed filepath, or that they are somehow used together.

lukpueh · 2021-10-26T12:47:35Z

tuf/ngclient/updater.py

+        If the ``target_dir`` argument was given to constructor, ``filepath``
+        argument is not required here: in this case a filename will be
+        generated based on the target path like in ``download_target()``


Maybe a slightly more detailed filepath arg description would eliminate the need for this paragraph here? See below for a suggestion.

tuf/ngclient/updater.py

Otherwise absolutely everything is split on multiple lines. Signed-off-by: Jussi Kukkonen <[email protected]>

Remove updated_targets() as it doesn't fit the rest of the API. In its stead add find_cached_target() which has a similar signature as download_target(): both accept an optional local filepath as argument and return full local filepath. In the find_cached_target() case None is returned if the local file is not the correct target file. Updater constructor gets a new optional target_dir argument: This means client can avoid giving a local filepath as an argument to find_cached_target()/download_target() -- Updater will then generate a filename within targets_dir. A reasonable use pattern (when targets_dir is set in constructor): info = updater.get_one_valid_targetinfo("targetname") path = updater.find_cached_target(info) if path is None: path = updater.download_target(info) Signed-off-by: Jussi Kukkonen <[email protected]>

This is slightly cosmetic but rename get_one_valid_targetinfo to get_targetinfo: * The function name is long without any reason: "one" and "valid" are always implicit * shortening makes code (incl. our examples and tests) easier to read * We're also already changing updater API (compared to legacy) so this alone does not break things -- it's also not a difficult "port". Signed-off-by: Jussi Kukkonen <[email protected]>

Also tweak the docstrings: the "caching" target_dir usage is presented in the module doc example: there should be no need for additional comments in the methods themselves as long as the argument docs are readable. Signed-off-by: Jussi Kukkonen <[email protected]>

jku · 2021-10-27T07:19:42Z

Rebased to get some test fixes from develop: there are no changes in the existing commits
added a new commit with the suggested refactor (added _generate_target_file_path()) and docstring tweaks

lukpueh

Thanks for addressing my comments, @jku, they LGTM! Let me know if you also want me to review the tests. Otherwise, and given @sechkova's and @MVrachev's approvals, I think this is good to go.

jku force-pushed the ngclient-api-polish branch from fe8bfb0 to 9bb0acd Compare October 5, 2021 13:24

jku force-pushed the ngclient-api-polish branch 2 times, most recently from 3870e46 to b984ec5 Compare October 6, 2021 08:56

jku marked this pull request as ready for review October 6, 2021 08:59

MVrachev self-requested a review October 6, 2021 13:43

jku mentioned this pull request Oct 7, 2021

ngclient: reveal final form (finish API changes) #1580

Closed

sechkova approved these changes Oct 7, 2021

View reviewed changes

tuf/ngclient/updater.py Outdated Show resolved Hide resolved

MVrachev reviewed Oct 7, 2021

View reviewed changes

jku force-pushed the ngclient-api-polish branch from b984ec5 to b56c363 Compare October 8, 2021 06:37

MVrachev approved these changes Oct 8, 2021

View reviewed changes

jku marked this pull request as draft October 13, 2021 08:21

jku force-pushed the ngclient-api-polish branch 2 times, most recently from 1a88493 to 3077e15 Compare October 21, 2021 11:00

jku force-pushed the ngclient-api-polish branch from 3077e15 to f6c10ad Compare October 21, 2021 11:41

jku marked this pull request as ready for review October 21, 2021 11:47

lukpueh reviewed Oct 26, 2021

View reviewed changes

jku mentioned this pull request Oct 27, 2021

ngclient: review argument names #1638

Closed

Jussi Kukkonen added 3 commits October 27, 2021 09:55

tests: Shorten variable names to reasonable length

cc9f387

Otherwise absolutely everything is split on multiple lines. Signed-off-by: Jussi Kukkonen <[email protected]>

jku force-pushed the ngclient-api-polish branch from f6c10ad to b3dc21e Compare October 27, 2021 07:16

jku force-pushed the ngclient-api-polish branch from b3dc21e to 6aaa1ea Compare October 27, 2021 07:19

lukpueh reviewed Oct 27, 2021

View reviewed changes

jku merged commit 7b8ff22 into theupdateframework:develop Oct 27, 2021

MVrachev mentioned this pull request Nov 5, 2021

ngclient updater: API functions argument validation #1594

Closed

4 tasks

lukpueh mentioned this pull request Dec 13, 2021

Prepare v0.20.0 #1723

Closed

jku deleted the ngclient-api-polish branch December 30, 2024 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ngclient api polish #1604

Ngclient api polish #1604

jku commented Oct 5, 2021 •

edited

Loading

joshuagl commented Oct 5, 2021

sechkova commented Oct 5, 2021 •

edited

Loading

jku commented Oct 5, 2021 •

edited

Loading

sechkova commented Oct 5, 2021

coveralls commented Oct 5, 2021 •

edited

Loading

jku commented Oct 6, 2021 •

edited

Loading

sechkova left a comment

MVrachev left a comment

MVrachev Oct 7, 2021

jku Oct 7, 2021

lukpueh Oct 26, 2021

lukpueh Oct 26, 2021

lukpueh Oct 26, 2021

jku commented Oct 8, 2021

MVrachev left a comment

sechkova commented Oct 12, 2021

jku commented Oct 21, 2021

jku commented Oct 21, 2021

lukpueh left a comment

lukpueh Oct 26, 2021

lukpueh Oct 26, 2021

lukpueh Oct 26, 2021

jku commented Oct 27, 2021 •

edited

Loading

lukpueh left a comment •

edited

Loading

Ngclient api polish #1604

Ngclient api polish #1604

Conversation

jku commented Oct 5, 2021 • edited Loading

joshuagl commented Oct 5, 2021

sechkova commented Oct 5, 2021 • edited Loading

jku commented Oct 5, 2021 • edited Loading

sechkova commented Oct 5, 2021

coveralls commented Oct 5, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1367591861

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

jku commented Oct 6, 2021 • edited Loading

sechkova left a comment

Choose a reason for hiding this comment

MVrachev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku commented Oct 8, 2021

MVrachev left a comment

Choose a reason for hiding this comment

sechkova commented Oct 12, 2021

jku commented Oct 21, 2021

jku commented Oct 21, 2021

lukpueh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jku commented Oct 27, 2021 • edited Loading

lukpueh left a comment • edited Loading

Choose a reason for hiding this comment

jku commented Oct 5, 2021 •

edited

Loading

sechkova commented Oct 5, 2021 •

edited

Loading

jku commented Oct 5, 2021 •

edited

Loading

coveralls commented Oct 5, 2021 •

edited

Loading

jku commented Oct 6, 2021 •

edited

Loading

jku commented Oct 27, 2021 •

edited

Loading

lukpueh left a comment •

edited

Loading