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

Fix version validation against chef's required format #1715

Merged
merged 3 commits into from
Jan 18, 2018

Conversation

robbkidd
Copy link
Contributor

@robbkidd robbkidd commented Jan 18, 2018

A version for a cookbook must be either X.Y.Z or X.Y. Up until this change, the validation for a CookbookVersion's version was against SemVer rules. SemVer allows for release labels or build info to appear after the version number itself. Chef does not!

Here, we add a new ChefVersionValidator that reuses the existing version validation logic of the Chef gem in ::Chef::Version. Supermarket is already relying on the Chef gem to validate version constraints set for cookbook dependencies and supported platform.

Fixes #1714 to return an error to any client if a cookbook is uploaded with an invalid version.

@robbkidd robbkidd requested a review from a team January 18, 2018 17:16
A version for a cookbook must be either X.Y.Z or X.Y.[1] Up until this
change, the validation for a CookbookVersion's version was against
SemVer rules. SemVer allows for release labels or build info to appear
after the version number itself. Chef does not!

[1] https://docs.chef.io/cookbook_versions.html#constraints

Here, we add a new ChefVersionValidator that reuses the existing version
validation logic of the Chef gem in ::Chef::Version. Supermarket is
already relying on the Chef gem to validate version constraints set for
cookbook dependencies and supported platform.

Fixes #1714 to return an error to any client if a cookbook is uploaded
with an invalid version.

Because cookbook versions are _Chef_ and not _Semantic_ versions, remove
the Semverse dependency altogether and use Chef::Version instead.

Signed-off-by: Robb Kidd <[email protected]>
In an effort to leave the camp site better than I found it, I'm
replacing these direct uses of Tempfile and AndFeathers with the spec
helper method that wraps this very thing.

Signed-off-by: Robb Kidd <[email protected]>
This allows use of binding.pry in tests.

Signed-off-by: Robb Kidd <[email protected]>
@robbkidd robbkidd force-pushed the robb/validate-cookbook-version-against-chef branch from b72b880 to ef8d518 Compare January 18, 2018 18:11
@robbkidd
Copy link
Contributor Author

robbkidd commented Jan 18, 2018

@thommay Because Supermarket is already depending on the Chef gem for Chef::VersionConstraint validation logic, is it sensible to double-down on the gem and use it for Chef::Version validation and comparison logic (instead of berkshelf/semverse)? Or do you foresee this leading down a dark road of pain and misery?

@lamont-granquist
Copy link
Contributor

This looks correct. Chef::Version is what we use to validate cookbook versions internally.

Copy link
Contributor

@thommay thommay left a comment

Choose a reason for hiding this comment

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

This looks sensible to me

@robbkidd robbkidd changed the title validate a cookbook version against chef's required format Fix version validation against chef's required format Jan 18, 2018
@robbkidd robbkidd merged commit f08a2eb into master Jan 18, 2018
@robbkidd robbkidd deleted the robb/validate-cookbook-version-against-chef branch January 18, 2018 18:36
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.

3 participants