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

semver: add wrapper for SemVer functionality #273

Merged
merged 1 commit into from Jul 18, 2019

Conversation

@ahmetb
Copy link
Member

commented Jul 17, 2019

This package introduces a wrapper for k8s.io/apimachinery/pkg/util/version
package for its parts that we rely on for our handling of semver values.

go.{mod,sum} will likely conflict soon as there are other in-flight PRs.

Also refactored the validation code to verify 'version' value can be parsed.

/assign @corneliusweig

@ahmetb

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2019

Tests expected to fail because kubernetes-sigs/krew-index#181 currently fails the krew search command.

@ahmetb ahmetb force-pushed the ahmetb:semver-parsing branch from 873f67a to 1ed7b8d Jul 17, 2019

@ahmetb ahmetb force-pushed the ahmetb:semver-parsing branch from 6beaa18 to f07b5a4 Jul 17, 2019

@k8s-ci-robot k8s-ci-robot removed the size/XL label Jul 17, 2019

@ahmetb ahmetb force-pushed the ahmetb:semver-parsing branch from f07b5a4 to bfe6f53 Jul 17, 2019

@corneliusweig
Copy link
Contributor

left a comment

Nice work overall, just some small nits.

func (v Version) String() string {
vv := k8sver.Version(v)
s := (&vv).String()
if !strings.HasPrefix(s, "v") {

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 17, 2019

Contributor

This check is obsolete, as it will always evaluate to true, so that could be a one-liner.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 18, 2019

Author Member

k8sver.Version#String() will return a string DEFINITELY starting without v. This check helps us change the underlying semver utility without changing our code. (This pkg is meant to be a wrapper.)

Not all semver parsers are enforcing v*, but I want to.

// character.
func Parse(s string) (Version, error) {
var vv Version
if !strings.HasPrefix(s, "v") {

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 17, 2019

Contributor

We could be lenient here and just pass through the semver, as k8sver understands an optional v prefix.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 18, 2019

Author Member

I think we should be less lenient first, then maybe more lenient.
Right now all plugins have v* prefix, so let's keep it?

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 18, 2019

Contributor

IMHO the k8sver handles errors just fine, so why add additional checks around that call? I don't think we will switch to another SemVer implementation which would require the v prefix (that would justify the additional check).
But it's a minor thing, I'm fine either way. Your call.

pkg/installation/semver/version_test.go Outdated Show resolved Hide resolved
{"negative value in minor", "v1.-2.3", "", true},
{"negative value in patch", "v1.2.-3", "", true},
{"empty pre-release identifier", "v1.0.1-", "", true},
{"empty meta identifier", "v1.0.1+", "", true},

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 17, 2019

Contributor

Maybe also worth testing if [:alpha:] chars lead to errors.

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 18, 2019

Author Member

can you give a specific example?

This comment has been minimized.

Copy link
@corneliusweig

corneliusweig Jul 18, 2019

Contributor

Something like

{"major with alpha chars", "va.0.1", "", true},
{"minor with alpha chars", "v1.a.1", "", true},
{"patch with alpha chars", "v1.0.a", "", true},

This comment has been minimized.

Copy link
@ahmetb

ahmetb Jul 18, 2019

Author Member

that feels a bit too obvious but I'll add tests for v0a.0.0, v0.0a.0 and v0.0.0a

@ahmetb ahmetb force-pushed the ahmetb:semver-parsing branch 7 times, most recently from 5be39c5 to c512307 Jul 18, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jul 18, 2019

Codecov Report

Merging #273 into master will increase coverage by 1.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #273      +/-   ##
==========================================
+ Coverage   53.94%   54.98%   +1.04%     
==========================================
  Files          18       19       +1     
  Lines         862      882      +20     
==========================================
+ Hits          465      485      +20     
  Misses        341      341              
  Partials       56       56
Impacted Files Coverage Δ
pkg/index/validate.go 95% <100%> (+0.26%) ⬆️
pkg/installation/semver/version.go 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8409b5...98fcd5a. Read the comment docs.

semver: add wrapper for SemVer functionality
This package introduces a wrapper for `k8s.io/apimachinery/pkg/util/version`
package for its parts that we rely on for our handling of semver values.

go.{mod,sum} will likely conflict soon as there are other in-flight PRs.

Also refactored the validation code to verify 'version' value can be parsed.

Signed-off-by: Ahmet Alp Balkan <ahmetb@google.com>

@ahmetb ahmetb force-pushed the ahmetb:semver-parsing branch from c512307 to 98fcd5a Jul 18, 2019

@corneliusweig

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 18, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link

commented Jul 18, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmetb, corneliusweig

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [ahmetb,corneliusweig]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 33b5c17 into kubernetes-sigs:master Jul 18, 2019

2 of 3 checks passed

tide Not mergeable. Needs lgtm label.
Details
cla/linuxfoundation ahmetb authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.