Repository: kubernetes-sigs/krew
Pull request #273 (2019-07-17 18:40:09)

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.

/assign @corneliusweig

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/273#" title="Author self-approved">ahmetb</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fkrew).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/krew/blob/master/OWNERS)~~ [ahmetb]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
k8s-ci-robot: @ahmetb: PR needs rebase.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
ahmetb: Tests expected to fail because https://github.com/kubernetes-sigs/krew-index/pull/181 currently fails the `krew search` command.
codecov-io: # [Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/273?src=pr&el=h1) Report
> Merging [#273](https://codecov.io/gh/kubernetes-sigs/krew/pull/273?src=pr&el=desc) into [master](https://codecov.io/gh/kubernetes-sigs/krew/commit/d8409b573daf42a6e386923915a237a485e3aaaf?src=pr&el=desc) will **increase** coverage by `1.04%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/kubernetes-sigs/krew/pull/273/graphs/tree.svg?width=650&token=J3nFtARI2m&height=150&src=pr)](https://codecov.io/gh/kubernetes-sigs/krew/pull/273?src=pr&el=tree)

```diff
@@            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](https://codecov.io/gh/kubernetes-sigs/krew/pull/273?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/index/validate.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/273/diff?src=pr&el=tree#diff-cGtnL2luZGV4L3ZhbGlkYXRlLmdv) | `95% <100%> (+0.26%)` | :arrow_up: |
| [pkg/installation/semver/version.go](https://codecov.io/gh/kubernetes-sigs/krew/pull/273/diff?src=pr&el=tree#diff-cGtnL2luc3RhbGxhdGlvbi9zZW12ZXIvdmVyc2lvbi5nbw==) | `100% <100%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/273?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/kubernetes-sigs/krew/pull/273?src=pr&el=footer). Last update [d8409b5...c512307](https://codecov.io/gh/kubernetes-sigs/krew/pull/273?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

corneliusweig: /lgtm
/approve
k8s-ci-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/krew/pull/273#" title="Author self-approved">ahmetb</a>*, *<a href="https://github.com/kubernetes-sigs/krew/pull/273#issuecomment-512943433" title="Approved">corneliusweig</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fkrew).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/kubernetes-sigs/krew/blob/master/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
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/go.sum b/go.sum
index 45edc08..97d57fd 100644
--- a/go.sum
+++ b/go.sum
@@ -120,6 +120,7 @@ k8s.io/client-go v7.0.0+incompatible h1:kiH+Y6hn+pc78QS/mtBfMJAMIIaWevHi++JvOGEE
 k8s.io/client-go v7.0.0+incompatible/go.mod h1:7vJpHMYJwNQCWgzmNV+VYUl1zCObLyodBc8nIyt8L5s=
 k8s.io/gengo v0.0.0-20190128074634-0689ccc1d7d6/go.mod h1:ezvh/TsK7cY6rbqRK0oQQ8IAqLxYwwyPxAX1Pzy0ii0=
 k8s.io/klog v0.0.0-20181102134211-b9b56d5dfc92/go.mod h1:Gq+BEi5rUBO/HRz0bTSXDUcqjScdoY3a9IHpCEIOOfk=
+k8s.io/klog v0.3.1 h1:RVgyDHY/kFKtLqh67NvEWIgkMneNoIrdkN0CxDSQc68=
 k8s.io/klog v0.3.1/go.mod h1:Gq+BEi5rUBO/HRz0bTSXDUcqjScdoY3a9IHpCEIOOfk=
 k8s.io/klog v0.3.3 h1:niceAagH1tzskmaie/icWd7ci1wbG7Bf2c6YGcQv+3c=
 k8s.io/klog v0.3.3/go.mod h1:Gq+BEi5rUBO/HRz0bTSXDUcqjScdoY3a9IHpCEIOOfk=
diff --git a/hack/make-release-artifacts.sh b/hack/make-release-artifacts.sh
index 46131d5..79b59e5 100755
--- a/hack/make-release-artifacts.sh
+++ b/hack/make-release-artifacts.sh
@@ -60,9 +60,15 @@ echo >&2 "Written ${zip_sumfile}."
 
 
 # Copy and process krew manifest
+git_describe="$(git describe --tags --dirty --always)"
+if [[ ! "${git_describe}" =~ v.* ]]; then
+  # if tag cannot be inferred (e.g. CI/CD), still provide a valid
+  # version field for krew.yaml
+  git_describe="v0.0.0-${git_describe}"
+fi
+krew_version="${TAG_NAME:-$git_describe}"
 cp ./hack/krew.yaml ./out/krew.yaml
-git_tag="${TAG_NAME:-$(git describe --tags --dirty --always)}"
 sed -i "s/KREW_ZIP_CHECKSUM/${zip_checksum}/g" ./out/krew.yaml
 sed -i "s/KREW_TAR_CHECKSUM/${tar_checksum}/g" ./out/krew.yaml
-sed -i "s/KREW_TAG/${git_tag}/g" ./out/krew.yaml
+sed -i "s/KREW_TAG/${krew_version}/g" ./out/krew.yaml
 echo >&2 "Written out/krew.yaml."
diff --git a/pkg/index/validate.go b/pkg/index/validate.go
index 1551024..45cb4bc 100644
--- a/pkg/index/validate.go
+++ b/pkg/index/validate.go
@@ -21,6 +21,7 @@ import (
 	"github.com/pkg/errors"
 
 	"sigs.k8s.io/krew/pkg/constants"
+	"sigs.k8s.io/krew/pkg/installation/semver"
 )
 
 var (
@@ -73,6 +74,9 @@ func (p Plugin) Validate(name string) error {
 	if p.Spec.Version == "" {
 		return errors.New("should have a version specified")
 	}
+	if _, err := semver.Parse(p.Spec.Version); err != nil {
+		return errors.Wrap(err, "failed to parse plugin version")
+	}
 	for _, pl := range p.Spec.Platforms {
 		if err := pl.Validate(); err != nil {
 			return errors.Wrapf(err, "platform (%+v) is badly constructed", pl)
diff --git a/pkg/installation/semver/version.go b/pkg/installation/semver/version.go
new file mode 100644
index 0000000..7208074
--- /dev/null
+++ b/pkg/installation/semver/version.go
@@ -0,0 +1,59 @@
+// Copyright 2019 The Kubernetes Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package semver is a wrapper for handling of semantic version
+// (https://semver.org) values.
+package semver
+
+import (
+	"strings"
+
+	"github.com/pkg/errors"
+	k8sver "k8s.io/apimachinery/pkg/util/version"
+)
+
+// Version is in-memory representation of a semantic version
+// (https://semver.org) value.
+type Version k8sver.Version
+
+// String returns string representation of a semantic version value with a
+// leading 'v' character.
+func (v Version) String() string {
+	vv := k8sver.Version(v)
+	s := (&vv).String()
+	if !strings.HasPrefix(s, "v") {
+		s = "v" + s
+	}
+	return s
+}
+
+// Parse parses a semantic version value with a leading 'v' character.
+func Parse(s string) (Version, error) {
+	var vv Version
+	if !strings.HasPrefix(s, "v") {
+		return vv, errors.Errorf("version string %q not starting with 'v'", s)
+	}
+	v, err := k8sver.ParseSemantic(s)
+	if err != nil {
+		return vv, err
+	}
+	return Version(*v), nil
+}
+
+// Less checks if a is strictly less than b (a<b).
+func Less(a, b Version) bool {
+	aa := k8sver.Version(a)
+	bb := k8sver.Version(b)
+	return aa.LessThan(&bb)
+}


Test patch:
diff --git a/pkg/index/validate_test.go b/pkg/index/validate_test.go
index c81cf97..e452908 100644
--- a/pkg/index/validate_test.go
+++ b/pkg/index/validate_test.go
@@ -22,6 +22,13 @@ import (
 	"sigs.k8s.io/krew/pkg/constants"
 )
 
+var (
+	defaultMeta = metav1.TypeMeta{
+		APIVersion: constants.CurrentAPIVersion,
+		Kind:       constants.PluginKind,
+	}
+)
+
 func Test_IsSafePluginName(t *testing.T) {
 	type args struct {
 		name string
@@ -107,10 +114,7 @@ func TestPlugin_Validate(t *testing.T) {
 		{
 			name: "success",
 			fields: fields{
-				TypeMeta: metav1.TypeMeta{
-					APIVersion: constants.CurrentAPIVersion,
-					Kind:       constants.PluginKind,
-				},
+				TypeMeta:   defaultMeta,
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: PluginSpec{
 					Version:          "v1.0.0",
@@ -176,10 +180,7 @@ func TestPlugin_Validate(t *testing.T) {
 		{
 			name: "shortDescription unspecified",
 			fields: fields{
-				TypeMeta: metav1.TypeMeta{
-					APIVersion: constants.CurrentAPIVersion,
-					Kind:       constants.PluginKind,
-				},
+				TypeMeta:   defaultMeta,
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: PluginSpec{
 					Version:          "v1.0.0",
@@ -199,10 +200,7 @@ func TestPlugin_Validate(t *testing.T) {
 		{
 			name: "version unspecified",
 			fields: fields{
-				TypeMeta: metav1.TypeMeta{
-					APIVersion: constants.CurrentAPIVersion,
-					Kind:       constants.PluginKind,
-				},
+				TypeMeta:   defaultMeta,
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: PluginSpec{
 					Version:          "",
@@ -220,12 +218,28 @@ func TestPlugin_Validate(t *testing.T) {
 			wantErr:    true,
 		},
 		{
-			name: "no file operations",
+			name: "version malformed",
 			fields: fields{
-				TypeMeta: metav1.TypeMeta{
-					APIVersion: constants.CurrentAPIVersion,
-					Kind:       constants.PluginKind,
+				TypeMeta:   defaultMeta,
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: PluginSpec{
+					Version:          "v01.02-a",
+					ShortDescription: "short",
+					Platforms: []Platform{{
+						URI:    "http://example.com",
+						Sha256: "deadbeef",
+						Files:  []FileOperation{{"", ""}},
+						Bin:    "foo",
+					}},
 				},
+			},
+			pluginName: "foo",
+			wantErr:    true,
+		},
+		{
+			name: "no file operations",
+			fields: fields{
+				TypeMeta:   defaultMeta,
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: PluginSpec{
 					Version:          "v1.0.0",
@@ -245,10 +259,7 @@ func TestPlugin_Validate(t *testing.T) {
 		{
 			name: "wrong plugin name",
 			fields: fields{
-				TypeMeta: metav1.TypeMeta{
-					APIVersion: constants.CurrentAPIVersion,
-					Kind:       constants.PluginKind,
-				},
+				TypeMeta:   defaultMeta,
 				ObjectMeta: metav1.ObjectMeta{Name: "wrong-name"},
 				Spec: PluginSpec{
 					Version:          "v1.0.0",
@@ -268,10 +279,7 @@ func TestPlugin_Validate(t *testing.T) {
 		{
 			name: "unsafe plugin name",
 			fields: fields{
-				TypeMeta: metav1.TypeMeta{
-					APIVersion: constants.CurrentAPIVersion,
-					Kind:       constants.PluginKind,
-				},
+				TypeMeta:   defaultMeta,
 				ObjectMeta: metav1.ObjectMeta{Name: "../foo"},
 				Spec: PluginSpec{
 					Version:          "v1.0.0",
diff --git a/pkg/installation/semver/version_test.go b/pkg/installation/semver/version_test.go
new file mode 100644
index 0000000..5a594b1
--- /dev/null
+++ b/pkg/installation/semver/version_test.go
@@ -0,0 +1,111 @@
+// Copyright 2019 The Kubernetes Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package semver
+
+import (
+	"fmt"
+	"testing"
+)
+
+func TestParse(t *testing.T) {
+	cases := []struct {
+		name    string
+		in      string
+		want    string
+		wantErr bool
+	}{
+		{"empty", "", "", true},
+		{"zero is valid", "v0.0.0", "v0.0.0", false},
+		{"no v prefix", "1.0.0", "", true},
+		{"spaces between v and value", "v 1.0.0", "", true},
+		{"major", "v1", "", true},
+		{"major.minor", "v1.2", "", true},
+		{"major.minor.patch", "v1.2.3", "v1.2.3", false},
+		{"major.minor.patch-suffix", "v1.2.3-beta.2+foo.bar", "v1.2.3-beta.2+foo.bar", false},
+
+		{"empty pre-release identifier", "v1.0.1-", "", true},
+		{"empty meta identifier", "v1.0.1+", "", true},
+		{"negative value in major", "v-1.2.3", "", true},
+		{"negative value in minor", "v1.-2.3", "", true},
+		{"negative value in patch", "v1.2.-3", "", true},
+		{"leading zero in major", "v01.2.3", "", true},
+		{"leading zero in minor", "v1.02.3", "", true},
+		{"leading zero in patch", "v1.2.03", "", true},
+		{"major with alpha chars", "v0a.0.0", "", true},
+		{"minor with alpha chars", "v0.0a.0", "", true},
+		{"patch with alpha chars", "v0.0.0a", "", true},
+	}
+	for _, tt := range cases {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := Parse(tt.in)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("Parse(%s) error = %v, wantErr %v", tt.in, err, tt.wantErr)
+				return
+			}
+			if err != nil {
+				return
+			}
+			if got.String() != tt.want {
+				t.Errorf("Parse(%s) = %q, want %q", tt.in, got.String(), tt.want)
+			}
+		})
+	}
+}
+
+func TestLess(t *testing.T) {
+	tests := []struct {
+		a      string
+		b      string // b≥a
+		equals bool
+	}{
+		{"v0.1.2", "v0.1.2", true},                 // equals
+		{"v1.0.0-alpha", "v1.0.0-alpha+foo", true}, // equals
+		{"v1.0.0-0.3.7", "v1.0.0-alpha", false},
+		{"v1.0.0-alpha", "v1.0.0-alpha.1", false},
+		{"v1.0.0-alpha.1", "v1.0.0-alpha.2", false},
+		{"v1.0.0-alpha.2", "v1.0.0-alpha.a", false},
+		{"v1.0.0-alpha", "v1.0.0-beta", false},
+		{"v1.0.1", "v1.0.2", false},
+		{"v1.0.1", "v1.2.0", false},
+		{"v1.0.1", "v2.1.0", false},
+		{"v1.0.0-alpha.2", "v1.0.1-alpha.1", false},
+	}
+	for _, tt := range tests {
+		t.Run(fmt.Sprintf("%s<%s", tt.a, tt.b), func(t *testing.T) {
+			va, err := Parse(tt.a)
+			if err != nil {
+				t.Fatalf("error parsing version %q", tt.a)
+			}
+			vb, err := Parse(tt.b)
+			if err != nil {
+				t.Fatalf("error parsing version %q", tt.b)
+			}
+
+			if tt.equals {
+				if o1, o2 := Less(va, vb), Less(vb, va); o1 || o2 {
+					t.Errorf("Less(%s,%s)=%v and Less(%s,%s)=%v; but they both should be false since the values are equal", tt.a, tt.b, o1, tt.b, tt.a, o2)
+				}
+			} else {
+				if !Less(va, vb) {
+					t.Errorf("Less(%s,%s) returned false; expected true", tt.a, tt.b)
+					return
+				}
+				if Less(vb, va) {
+					t.Errorf("(flipped) Less(%s,%s) returned true; expected false", tt.b, tt.a)
+				}
+			}
+		})
+	}
+}