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

New metadata API: spec_version attribute validation #1430

Merged
merged 2 commits into from
Jun 16, 2021

Conversation

MVrachev
Copy link
Collaborator

@MVrachev MVrachev commented Jun 1, 2021

Fixes #1419

Description of the changes being introduced by the pull request:
Even though version strings like 2.0.0-rc.2 or 1.0.0-beta are
valid strings in semantic versioning format, in TUF we never needed
to add letters for our specification number.

That's why I validate that: spec_version is a . separated string
and when split it has a length of 3 and that each of the
three elements is a number.

Also, I remove one comment about the version validation place, because it
seems irrelevant anymore.

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

@MVrachev MVrachev changed the title Spec version validation New metadata API: spec_version attribute validation Jun 1, 2021
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm approving, just commenting:

  • referring to semver in error message is maybe a bit misleading (when we only accept a small but common subset of semver )
  • I'm fine with being more strict than the spec when it's useful, I just don't quite get why we do that here

anyway, those are minor quibbles: LGTM, thanks

@MVrachev
Copy link
Collaborator Author

MVrachev commented Jun 2, 2021

I'm approving, just commenting:

  • referring to semver in error message is maybe a bit misleading (when we only accept a small but common subset of semver )

Yea, you are right, but if I wanted to be 100% correct, I will have to use more words and make this message longer...
Considering we don't need letters for our spec version, for now, I think this message is okay even if a little misleading.

  • I'm fine with being more strict than the spec when it's useful, I just don't quite get why we do that here

I think it's better to be more conservative when we do checks than liberal and given that we will use almost the same amount of code if we check that the first element in the string is a number or all of them I decided the latter.
Also, the semver format specifically mentions that all of the three dot-separated strings are positive integers: https://semver.org/#spec-item-2 and if there is a need one could add an additional string to the third one.

@MVrachev
Copy link
Collaborator Author

MVrachev commented Jun 2, 2021

@jku I realized that probably before merging we would want to create some tests actually testing spec_version invalid examples?
That applies to version and expiry even though we haven't made changes to their code.
That way when we finish with the initial discussion for each of the attributes we would know that we have added some testing as well.

@jku
Copy link
Member

jku commented Jun 2, 2021

I think it's better to be more conservative when we do checks than liberal

We're spending time writing code that prevents future spec writers from using something like "2.0.0.beta1" without getting any benefits that I can see -- but it's fine let's move on.

@jku
Copy link
Member

jku commented Jun 2, 2021

Oh I forgot: Did you actively decide not to verify the actual version number against version supported by the implementation (I may have missed the discussion)?

Maybe we do what the old implementation did: raise if major version does not match the supported major version (which I think should probably be defined in tuf/api/metadata.py and not tuf/__init__.py considering we have essentially two implementations)?

@MVrachev MVrachev force-pushed the spec_version-validation branch from a138869 to 08e8701 Compare June 3, 2021 15:23
@MVrachev
Copy link
Collaborator Author

MVrachev commented Jun 3, 2021

Oh I forgot: Did you actively decide not to verify the actual version number against version supported by the implementation (I may have missed the discussion)?

Maybe we do what the old implementation did: raise if major version does not match the supported major version (which I think should probably be defined in tuf/api/metadata.py and not tuf/__init__.py considering we have essentially two implementations)?

Great catch!
I updated the pr.

@MVrachev MVrachev force-pushed the spec_version-validation branch from 08e8701 to adca75f Compare June 3, 2021 15:28
@jku jku self-requested a review June 4, 2021 11:26
@MVrachev MVrachev force-pushed the spec_version-validation branch 4 times, most recently from 34da1ff to 69c4385 Compare June 7, 2021 10:44
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit grumpy about +19 lines of code for something like spec_version but I guess the comments are mostly suggestions -- I'm not seeing bugs or demanding changes.

The SPECIFICATION_VERSION issue I can live with as well, I just don't understand why we would want to do that.

tuf/api/metadata.py Outdated Show resolved Hide resolved
tuf/api/metadata.py Outdated Show resolved Hide resolved
tuf/api/metadata.py Outdated Show resolved Hide resolved
@MVrachev MVrachev force-pushed the spec_version-validation branch 2 times, most recently from 56d011e to 7bb8b4a Compare June 8, 2021 14:25
In issue theupdateframework#1418 in this comment:
theupdateframework#1418 (comment)
I summarized the discussion we had with the participants in this issue.
In summary: no additional changes are needed for "version" validation
considering there is "bump_version()" function for that.

If we won't be adding "version" validation elsewhere we can keep it
the way it is.

Signed-off-by: Martin Vrachev <[email protected]>
@MVrachev MVrachev force-pushed the spec_version-validation branch from 7bb8b4a to 26c2e8d Compare June 9, 2021 10:54
@jku jku self-requested a review June 14, 2021 06:57
@jku
Copy link
Member

jku commented Jun 14, 2021

code looks great to me (thanks) but I have reservations on the comment, placement and value of SPECIFICATION_VERSION:

  • I don't understand why we would want to specify 1.0.0 as version specifically (I could understand either specifying MAJOR only or specifying the latest specific version we think we're up-to-date with -- specifying 1.0.0 definitely gives the wrong message to me)
  • what the comment about "conforming" actually means and does it really need six lines
  • why the value couldn't be in metadata.py (which is likely the only place that will need it).

@MVrachev MVrachev force-pushed the spec_version-validation branch from 26c2e8d to 24a39c2 Compare June 14, 2021 14:48
@MVrachev
Copy link
Collaborator Author

Updated where I placed the SPECIFICATION_VERSION constant, used 1.0.19 as supported, and shorten the comment.

According to point 2 in the semver specification:
"A normal version number MUST take the form X.Y.Z where X, Y, and Z are
non-negative integers...". See: https://semver.org/#spec-item-2
Also, even though version strings like "2.0.0-rc.2" or "1.0.0-beta" are
valid strings in semantic versioning format, in TUF we never needed
to add letters for our specification number.
That's why I validate that: spec_version is a . separated string
and when split it has a length of 3 and that each of the
three elements is a number.

The modules under the tuf/api folder in TUF are an alternative TUF
implementation. That's why they should use their own constant for
SPECIFICATION_VERSION in tuf/metadata/api.

This time, I used a list for the SPECIFICATION_VERSION constant in order
to retrieve major and minor versions easier.

I use the SPECIFICATION_VERSION to check that the given spec_version is
supported against the tuf code spec version.

Signed-off-by: Martin Vrachev <[email protected]>
@MVrachev MVrachev force-pushed the spec_version-validation branch from 24a39c2 to 41afb1e Compare June 14, 2021 14:58
Copy link
Member

@jku jku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you

@jku jku merged commit 25e5f30 into theupdateframework:develop Jun 16, 2021
@MVrachev MVrachev deleted the spec_version-validation branch July 19, 2021 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metadata Attribute research: spec_version
2 participants