-
Notifications
You must be signed in to change notification settings - Fork 278
Version Management #854
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
Version Management #854
Conversation
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.
It might be worth taking a look at version parsing libraries? distutils.version
is in the standard library and should do the trick.
Or the third-party packaging.version
, if PEP-440 compliance is required.
What's the goal here? Existing code on the develop branch already does this comparison. This adds some temp variables for minor and fix, only if they exist, and then checks them (whether or not they existed). |
I updated it to use parse_version which eliminates the temp variables and makes it a bit cleaner. The goal is to report to the user if the minor versions do not match. The update can still continue if the major versions are the same, but this allows users to know when their client is out of date. This is a small change to make the reference implementation consistent with a proposed TAP (https://github.com/theupdateframework/taps/pull/107/files) |
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.
Thanks for the patch. See my comments inline.
Please also consider reading our dev guidelines, especially the parts about uniform descriptive commit messages. They are very helpful for reviewers. :)
Thanks!
tuf/client/updater.py
Outdated
@@ -1495,14 +1496,21 @@ def _get_metadata_file(self, metadata_role, remote_filename, | |||
metadata_spec_version = metadata_signable['signed']['spec_version'] | |||
metadata_spec_major_version = int(metadata_spec_version.split('.')[0]) | |||
code_spec_major_version = int(tuf.SPECIFICATION_VERSION.split('.')[0]) | |||
|
|||
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.
Please remove trailing whitespace
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.
OT: We might want to activate a corresponding lint policy (see in-toto's pylintrc).
"spec_version. This code has version " + str(tuf.SPECIFICATION_VERSION) + | ||
"and the metadata lists version number " + str(metadata_spec_version) + | ||
". The update will continue as the major versions match.") | ||
|
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.
Please try to wrap lines at character 79.
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.
OT: This is actually something we should have the linter check as well.
tuf/client/updater.py
Outdated
if metadata_spec_major_version != code_spec_major_version: | ||
raise tuf.exceptions.UnsupportedSpecificationError( | ||
'Downloaded metadata that specifies an unsupported ' | ||
'spec_version. This code supports major version number: ' + | ||
repr(code_spec_major_version) + '; however, the obtained ' | ||
'metadata lists version number: ' + str(metadata_spec_version)) | ||
|
||
#report to user if minor versions do not match, continue with update | ||
if pkg_resources.parse_version(metadata_spec_version) != pkg_resources.parse_version(tuf.SPECIFICATION_VERSION): |
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.
If you compare the full versions you don't need to parse them. But if I understand correctly, you don't want to compare the full version, just the minor version. The major version you already compare above and patch/fix version you don't care about.
Also, your log message says that one version (part) is higher than the other, so you will need to compare with >
operator.
I suggest you parse both metadata_spec_version
and tuf.SPECIFICATION_VERSION
only once and then use the major parts for the first (existing) check and the minor parts for the second (your new) check.
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.
Thanks or addressing my comments. I have two more minor concerns. Also you have trailing whitespace in three lines, consider taking a look at secure-systems-lab/code-style-guidelines#5, which describes how to detect them.
if metadata_spec_major_version != code_spec_major_version: | ||
raise tuf.exceptions.UnsupportedSpecificationError( | ||
'Downloaded metadata that specifies an unsupported ' | ||
'spec_version. This code supports major version number: ' + | ||
repr(code_spec_major_version) + '; however, the obtained ' | ||
'metadata lists version number: ' + str(metadata_spec_version)) | ||
|
||
#report to user if minor versions do not match, continue with update |
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.
The comment suggests !=
but the code and the log message >
. Which one is it?
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 updated the operator and message to report any mismatch in the minor version.
tuf/client/updater.py
Outdated
logger.info("Downloaded metadata that specifies a higher minor " + | ||
"spec_version. This code has version " + | ||
str(tuf.SPECIFICATION_VERSION) + | ||
"and the metadata lists version number " + |
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 you are missing a whitespace before and
.
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.
LGTM! Please run the necessary git commands (e.g. as suggested by probot) to "sign them off" retroactively.
Signed-off-by: marinamoore <[email protected]>
Signed-off-by: marinamoore <[email protected]>
Signed-off-by: marinamoore <[email protected]>
Signed-off-by: marinamoore <[email protected]>
Signed-off-by: marinamoore <[email protected]>
Signed-off-by: marinamoore <[email protected]>
Signed-off-by: marinamoore <[email protected]>
Signed-off-by: marinamoore <[email protected]>
…versions Signed-off-by: marinamoore <[email protected]>
Signed-off-by: marinamoore <[email protected]>
2495062
to
149d5bd
Compare
Please fill in the fields below to submit a pull request. The more information
that is provided, the better.
Fixes issue #:
Reference implementation for Version Management TAP (https://github.com/theupdateframework/taps/pull/107/files)
Description of the changes being introduced by the pull request:
This pull request adds a comparison of minor spec_versions (in format major.minor.fix) during an update and reports an out of date client to that client. Minor versions will not have breaking changes, so the update may continue.
Please verify and check that the pull request fulfills the following
requirements: