Skip to content
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

client refactor: consider API changes #1317

Closed
jku opened this issue Mar 18, 2021 · 7 comments
Closed

client refactor: consider API changes #1317

jku opened this issue Mar 18, 2021 · 7 comments
Labels
backlog Issues to address with priority for current development goals ngclient

Comments

@jku
Copy link
Member

jku commented Mar 18, 2021

We should consider if we want to additions/changes to the API -- this would be a good time for them.

Possibilities:

1. Namespace shortening: currently using the client looks like this:

from tuf.client import updater
updater = updater.Updater() # that looks stupid and is a name clash

This is a small thing maybe but having both client and updater module does not seem useful: I think we could just as well have

from tuf import client
updater = client.Updater()

This should be possible by having a client/__init__.py that exports Updater (and FetcherInterface).

2. Mirrors: this is arguably the most complex part of the client API, probably mostly unused and is kind of hidden: I wonder if we can/should make it better? pip does need the mirrors config but even that use is not what mirrors config was designed for: pip potentially changes the config before every single new target download, switching the mirror list based on whether it's downloading index files or distribution files...
#1307, #1143

3. the actual updater API do we want to improve the get_one_valid_targetinfo + updated_targets + download_target call sequence in any way?
* Possibly provide a helper that does the obvious thing (given a target name, just make sure it is downloaded)
* should the actual local paths be returned? It feels odd that client has to build the paths when updater already had them

@jku jku added the experimental-client Items related to the development of a new client (see milestone/8 and theexperimental-client branch) label Mar 18, 2021
@joshuagl
Copy link
Member

  1. I'm 💯 for the namespace shortening.
  2. I do want us to think about the mirrors, it's a popular topic Split Updater into logical components: redesign mirrors.py and download.py #1307 and client/updater design: mirror config redesign #1143.
  3. I like both of these suggestions, we also have Design client library #1135 has some additional thoughts on client API

@mnm678
Copy link
Contributor

mnm678 commented Mar 18, 2021

I'm not sure if we should design for not-yet-accepted TAPs (we probably shouldn't), but there might be a way to combine the mirrors functionality with the client configuration in TAP 13 (#1103) and possibly also TAP 4 into a more general 'configurations' API, where the client could set these options before using client.Updater.

@jku
Copy link
Member Author

jku commented Mar 31, 2021

Another thing I've not mentioned: currently it's not possible to download without writing to a file. Usually that's fine but the pip index files might be a use case that would benefit from doing that: the code currently opens and read()s the file immediately after download succeeds...

@sechkova sechkova added this to the Client Refactor milestone Apr 7, 2021
@jku
Copy link
Member Author

jku commented May 19, 2021

So number 1 is handled in #1397 in coming days, item 2 was solved by not supporting mirrors.

I have some more specific ideas about item 3:

  • the current API is needlessly complex with a self defined dict returned from get_on_valid_targeinfo and expected as argument in otherfunctions : we should probably just return a TargetFile object from get_one_valid_targetinfo() or whatever we call it
  • I'm seriously considering if Updater on its own should not support target caching at all -- it's not part of the spec, it has these sharp corners WRT urls being used as file paths, and I think it could be handled as well by e.g. a derived CachingUpdater

Non-caching updater could look like

class Updater:
    def __init__(...)
    def refresh()
    def find_target_file(name:str ) -> TargetFile
    # here caller can use TargetFile.verify_length_and_hashes(file_obj) if they want to check a local file
    def download_target(target: TargetFile, local_path: str)

The CachingUpdater would add:

    def is_target_cached(target: TargetFile) -> bool
    def download_target(target: TargetFile) -> str

Note that this assumes TargetFile actually included the target name. If it does not then several methods here still need another argument -- but I think we should consider if TargetFile (and possibly MetaFile) should actually include the name

None of this needs to happen before merging to develop: they should not lead to major code movements or rewrites

@jku jku added ngclient and removed experimental-client Items related to the development of a new client (see milestone/8 and theexperimental-client branch) labels May 21, 2021
@sechkova sechkova added the backlog Issues to address with priority for current development goals label May 26, 2021
@joshuagl joshuagl removed this from the Experimental client milestone May 26, 2021
@MVrachev
Copy link
Collaborator

  1. Mirrors:

Fixed by #1408 wherein the new client implementation we drop mirrors support.

the current API is needlessly complex with a self defined dict returned from get_on_valid_targeinfo and expected as argument in otherfunctions : we should probably just return a TargetFile object from get_one_valid_targetinfo() or whatever we call it

Fixed by #1514

Another thing I've not mentioned: currently it's not possible to download without writing to a file. Usually that's fine but the pip index files might be a use case that would benefit from doing that: the code currently opens and read()s the file immediately after download succeeds...

What are pip index files and how do they relate to the target downloads?
Is this something we want to consider? If so maybe we need an issue about it?

Besides the last point, I mentioned above, am I correct that as TODO items required to solve this issue are:

@jku
Copy link
Member Author

jku commented Aug 28, 2021

What are pip index files and how do they relate to the target downloads?

pages like this https://pypi.org/simple/sampleproject/. In TUF terminology this html is a target file.

Is this something we want to consider? If so maybe we need an issue about it?

Yeah, a new issue would be fine: this meta-issue could then be closed as all items are in actual issues.

This might make sense for two reasons:

  • since pip already caches things like this with the http cache, TUF does not need to do it
  • From pip point of view there's no need to write the index file to a local file as pip just wants to parse the content of the file and then forget about it. It's not awful if a file is written but it's not needed.

Besides the last point, I mentioned above, am I correct that as TODO items required to solve this issue are:

The first one of those is an internal detail of the Updater so not an API change at all.

@jku
Copy link
Member Author

jku commented Sep 1, 2021

I'm closing as individual ideas here now have issues of their own

@jku jku closed this as completed Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals ngclient
Projects
None yet
Development

No branches or pull requests

5 participants