Metadata API: include target target name in TargetFile #1514
Conversation
MVrachev
changed the title
tuf/api/metadata.py
Outdated
| unrecognized_fields: Dictionary of all unrecognized fields. | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| length: int, | ||
| hashes: Dict[str, str], | ||
| targetname: Optional[str] = None, |
There was a problem hiding this comment.
I'm not a fan ofTargetFile.targetname. How about TargetFile.filename, TargetFile.path or TargetFile.targetpath if we insist on being verbose and stick to the spec.
There was a problem hiding this comment.
(this is just a drive by comment, will have to think about the justification for thw whole thing a bit still... but thanks for starting this, I think it's useful to make discussion more concrete)
What teodora said, let's not use even more different names for the same thing... I think "targetpath" is used for this already so the attribute should probably be targetpath or path.
I believe it should not be optional though. Or is there a use case of a TargetFile without a targetpath?
There was a problem hiding this comment.
I'm not a fan of TargetFile.targetname. How about TargetFile.filename, TargetFile.path or TargetFile.targetpath if we insist on being verbose and stick to the spec.
I had a misconception and didn't want to call it targetpath thinking it's not an actual path, but a file name.
Now, I read the spec and it clearly this is called TARGETPATH:
https://theupdateframework.github.io/specification/latest/#targetpath.
So, from both options to call it targetpath or path I would prefer path given it's shorter.
I believe it should not be optional though. Or is there a use case of a TargetFile without a targetpath?
I wasn't sure when I added it as optional. I did that because it's not required by the spec, but we can use TargetFile
in such a way that we would always pass the path.
So, in conclusion, I will rename the attribute to path and make it non-optional.
MVrachev
force-pushed
the
filename-in-targetfile
branch
from
ef5cce9
to
3c4d5dd
Compare
|
I think this makes sense and implementation looks fine. You will have to do something about Maybe it would be good to document target path a little more: that it is a "path-relative-URL string" that will be used as part of the final download url to fetch the target The high level question is whether anyone objects to making objects like this a bit smarter (in this case be aware of the path) even if the file format does not include that. I think this is a good way forward and seems immediately understandable by the reader -- it definitely makes the Updater API much better... |
|
For those not following that closely: this PR changes TargetFile interface (which on it's own is a minor change in the Metadata API implementation) but also the whole ngclient API so that TargetFile is returned and expected as argument instead of an undefined dictionary. I think this is going in the right direction and allows us to continue with something like #1317 (comment): I believe the core Updater should not handle caching: this would make it much clearer what is actually part of the spec implementation and what is an additional thing we just wanted to implement in the same source directory |
I do not object, in fact looking at it now having an object which describes a file without being able to explicitly identify the file does look odd. +1 from me. |
|
@MVrachev can you suggest a solution for |
MVrachev
force-pushed
the
filename-in-targetfile
branch
from
3c4d5dd
to
4969ad3
Compare
|
I had to rebase to fix merge conflicts after the simplification of |
MVrachev
force-pushed
the
filename-in-targetfile
branch
from
4969ad3
to
440040c
Compare
MVrachev
force-pushed
the
filename-in-targetfile
branch
from
440040c
to
a98bdc0
Compare
Currently, TargetFile instances do not contain the path relative URL of the file they represent. The API itself does not need it but it could be useful for users of the API. As an example, the current client returns a dict for get_one_valid_targetinfo(): that dict contains a filepath field and a targetinfo field (essentially TargetFile). We would like to keep a similar API, but avoid hand-crafted dicts. It would be much nicer to return a TargetFile that would contain the full "metadata" of the targetfile. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
After the addition of "path" argument in the TargetFile class the filename argument in Targets.update() became redundant. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
MVrachev
force-pushed
the
filename-in-targetfile
branch
from
a98bdc0
to
9229a40
Compare
Related to #1411, but it's not yet clear if it closes it.
Description of the changes being introduced by the pull request:
Currently, TargetFile instances do not contain the filename of the file
they represent. The API itself does not need it but it could be useful
for users of the API.
As an example, the current client returns a dict for
get_one_valid_targetinfo(): that dict contains a filepath field anda targetinfo field (essentially TargetFile).
We would like to keep a similar API, but avoid hand-crafted dicts.
It would be much nicer to return a TargetFile that would contain the
full "metadata" of the targetfile.
Signed-off-by: Martin Vrachev mvrachev@vmware.com
Please verify and check that the pull request fulfills the following
requirements: