Refactor _get_metadata_file() from updater.py #1205
Conversation
e21d343 to
3f154b3
Compare
There was a problem hiding this comment.
There's probably a debate here: do we want to do fixes like this now, does this make e.g. Teodoras work more difficult?
Personally I think there's value in this and the idea is correct: it makes the core update loop easier to read and makes us find issues in it. That said, the update loop feels brittle: let's make sure we do not break it... I left comments in code.
I also think we should try to fix the comments and logging when we touch them (make sure they're correct, useful and not too verbose) -- did not leave comments on those yet
It will be best if @sechkova could look into those changes and give an opinion as well.
Yes, a good reminder. |
|
I am by no means against improving the code given that this is the currently used version and the rework is only just a dream ... yet. |
From memory: expected_version argument would have the typing hint "Optional[int]" but the possibility of None is not documented in several function docstrings. Both |
dc1e0c2 to
ab51493
Compare
ab51493 to
d6f061f
Compare
Currently, the `_get_metadata_file()` from `tuf/client/updater.py` is really large and does too many things and some of them partially. Right now, it validates the specification version and the metadata version in it, but doesn't validate if the metadata has expired or if the signature could be trusted. So, it's partially validating the metadata file and at the same time calling `_verify_metadata_file()` from updater.py for the rest of the work which doesn't make sense. It's logically better if we move the validation of the specification and the metadata version in separate functions which would be then called in ` _verify_metadata_file()` and that way all of the validation will be done in ` _verify_metadata_file()`. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
d6f061f to
c8f0c1c
Compare
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
|
After a discussion with @joshuagl, I added the last commit to simplify some of the comments and the way we check for spec conformance. |
Description of the changes being introduced by the pull request:
Currently, the
_get_metadata_file()fromtuf/client/updater.pyis really largeand does too many things and some of them partially.
It validates the specification version and the metadata version in it,
but doesn't validate if the metadata has expired or if the signature could be trusted.
So, it's partially validating the metadata file and at the same time calling
_verify_metadata_file()from updater.py for the rest of the work which doesn't makesense.
It's logically better if we move the validation of the specification
and the metadata version in separate functions which would
be then called in
_verify_metadata_file()and that way move all of the validationwill be done in
_verify_metadata_file().Also, this pr is needed if we want to validate locally available metadata files.
Currently, the problem is that part of the validation cannot be done for local files, because
the spec and metadata versions validation is done in
get_metadata_file()which isused for accessing a metada file somewhere on the internet.
Please verify and check that the pull request fulfills the following
requirements: