Version Management by mnm678 · Pull Request #854 · theupdateframework/python-tuf

mnm678 · 2019-04-12T20:23:33Z

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:

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

lukpueh

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.

trishankatdatadog · 2019-04-15T15:28:54Z

@lukpueh https://github.com/DataDog/integrations-core/blob/23ebd01428da80b85ad59d5c7e23be57a1970cee/datadog_checks_downloader/datadog_checks/downloader/download.py#L298-L299

awwad · 2019-04-15T16:01:21Z

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).

mnm678 · 2019-04-24T18:43:15Z

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)

lukpueh

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!

lukpueh · 2019-06-06T12:10:23Z

tuf/client/updater.py

          metadata_spec_major_version = int(metadata_spec_version.split('.')[0])
          code_spec_major_version = int(tuf.SPECIFICATION_VERSION.split('.')[0])
-
+          


Please remove trailing whitespace

OT: We might want to activate a corresponding lint policy (see in-toto's pylintrc).

lukpueh · 2019-06-06T12:12:52Z

tuf/client/updater.py

+                "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.")
+


Please try to wrap lines at character 79.

OT: This is actually something we should have the linter check as well.

lukpueh · 2019-06-06T12:24:15Z

tuf/client/updater.py

                '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):


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.

lukpueh

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.

lukpueh · 2019-06-06T18:27:39Z

tuf/client/updater.py

                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


The comment suggests != but the code and the log message >. Which one is it?

I updated the operator and message to report any mismatch in the minor version.

lukpueh · 2019-06-06T18:28:53Z

tuf/client/updater.py

+            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 " + 


I think you are missing a whitespace before and.

lukpueh

LGTM! Please run the necessary git commands (e.g. as suggested by probot) to "sign them off" retroactively.

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

…versions Signed-off-by: marinamoore <mmoore32@calpoly.edu>

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

lukpueh reviewed Apr 15, 2019

View reviewed changes

lukpueh requested changes Jun 6, 2019

View reviewed changes

lukpueh reviewed Jun 6, 2019

View reviewed changes

lukpueh approved these changes Jun 10, 2019

View reviewed changes

mnm678 added 10 commits June 10, 2019 10:18

added minor version check

5ec39d4

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

support shorter spec_version

1326a01

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

bug fix

9d05dde

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

use distutils.version

812170c

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

use parse_version

9e2b5c7

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

Updated the spec-version check to only parse the version once

45aa0b5

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

removed unused import

819a5dd

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

removed trailing whitespace

9d5eef1

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

minor spec-version comparison now checks for any difference in minor …

e7a49a8

…versions Signed-off-by: marinamoore <mmoore32@calpoly.edu>

removed trailing whitespace

149d5bd

Signed-off-by: marinamoore <mmoore32@calpoly.edu>

mnm678 force-pushed the version_management branch from 2495062 to 149d5bd Compare June 10, 2019 14:20

lukpueh merged commit 65e5ee1 into theupdateframework:develop Jun 19, 2019

lukpueh mentioned this pull request Sep 26, 2019

Documenting or creating a TAP to describe keyid_hash_algorithms #848

Closed

mnm678 deleted the version_management branch April 16, 2021 21:01

		metadata_spec_major_version = int(metadata_spec_version.split('.')[0])
		code_spec_major_version = int(tuf.SPECIFICATION_VERSION.split('.')[0])

Conversation

mnm678 commented Apr 12, 2019

Uh oh!

lukpueh left a comment

Choose a reason for hiding this comment

Uh oh!

trishankatdatadog commented Apr 15, 2019

Uh oh!

awwad commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnm678 commented Apr 24, 2019

Uh oh!

lukpueh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukpueh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukpueh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

awwad commented Apr 15, 2019 •

edited

Loading