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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions go.sum
Expand Up @@ -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=
Expand Down
10 changes: 8 additions & 2 deletions hack/make-release-artifacts.sh
Expand Up @@ -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."
4 changes: 4 additions & 0 deletions pkg/index/validate.go
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/pkg/errors"

"sigs.k8s.io/krew/pkg/constants"
"sigs.k8s.io/krew/pkg/installation/semver"
)

var (
Expand Down Expand Up @@ -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)
Expand Down
56 changes: 32 additions & 24 deletions pkg/index/validate_test.go
Expand Up @@ -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
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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: "",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down
59 changes: 59 additions & 0 deletions 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") {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@ahmetb ahmetb Jul 18, 2019

Choose a reason for hiding this comment

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

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.

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") {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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)
}
111 changes: 111 additions & 0 deletions 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)
}
}
})
}
}