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

Better API version parsing. #1020

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion cmd/skaffold/app/cmd/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package util

import (
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/apiversion"
yaml "gopkg.in/yaml.v2"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
Expand All @@ -35,10 +36,19 @@ func ParseConfig(filename string) (*config.SkaffoldConfig, error) {
return nil, errors.Wrap(err, "parsing api version")
}

if apiVersion.Version != config.LatestVersion {
parsedVersion, err := apiversion.ParseVersion(apiVersion.Version)
if err != nil {
return nil, errors.Wrap(err, "parsing api version")
}

if parsedVersion.LT(config.LatestAPIVersion) {
return nil, errors.New("Config version out of date: run `skaffold fix`")
}

if parsedVersion.GT(config.LatestAPIVersion) {
return nil, errors.New("Config version is too new for this version of skaffold: upgrade skaffold")
}

cfg, err := config.GetConfig(buf, true)
if err != nil {
return nil, errors.Wrap(err, "parsing skaffold config")
Expand Down
97 changes: 97 additions & 0 deletions cmd/skaffold/app/cmd/util/util_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/*
Copyright 2018 The Skaffold 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 util

import (
"io/ioutil"
"os"
"testing"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/schema/v1alpha1"

yaml "gopkg.in/yaml.v2"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/config"
)

func TestParseConfig(t *testing.T) {
type args struct {
apiVersion string
}
tests := []struct {
name string
args args
wantErr bool
}{
{
name: "good api version",
args: args{
apiVersion: config.LatestVersion,
},
wantErr: false,
},
{
name: "old api version",
args: args{
apiVersion: v1alpha1.Version,
},
wantErr: true,
},
{
name: "new api version",
args: args{
apiVersion: "skaffold/v9",
},
wantErr: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
cfg, err := config.NewConfig()
if err != nil {
t.Fatalf("error generating config: %s", err)
}
cfg.APIVersion = tt.args.apiVersion
cfgStr, err := yaml.Marshal(cfg)
if err != nil {
t.Fatalf("error marshalling config: %s", err)
}

p := writeTestConfig(t, cfgStr)
defer os.Remove(p)

_, err = ParseConfig(p)
if (err != nil) != tt.wantErr {
t.Errorf("ParseConfig() error = %v, wantErr %v", err, tt.wantErr)
return
}
})
}
}

func writeTestConfig(t *testing.T, cfg []byte) string {
t.Helper()
f, err := ioutil.TempFile("", "")
if err != nil {
t.Fatalf("error getting temp file: %s", err)
}
defer f.Close()
if _, err := f.Write(cfg); err != nil {
t.Fatalf("error writing config: %s", err)
}
return f.Name()
}
119 changes: 119 additions & 0 deletions pkg/skaffold/apiversion/apiversion.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
/*
Copyright 2018 The Skaffold 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 apiversion

import (
"fmt"
"regexp"
"strconv"
)

type ReleaseTrack int

const (
alpha ReleaseTrack = 0
beta ReleaseTrack = 1
ga ReleaseTrack = 2
)

type Version struct {
Major int
Minor int
Release ReleaseTrack
}

var re = regexp.MustCompile(`^skaffold/v(\d)(?:(alpha|beta)(\d))?$`)
r2d4 marked this conversation as resolved.
Show resolved Hide resolved

// ParseAPIVersion parses a string into a Version.
func ParseVersion(v string) (*Version, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an easier mapping to blang/semver

v1alpha3 -> 1.0.0-alpha.3
v1 -> 1.0.0

v2beta3 -> 2.0.0-beta.3
v2beta4 -> 2.0.0-beta.4

Copy link
Contributor

Choose a reason for hiding this comment

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

Once we map it to blang/semver, we can remove the comparison logic and just reuse that library's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's prefer to keep it this way rather than turn this into a semver and reparse with blang - I tried it and it's about the same amount of code. Let's see what others think though, I'm not sure one is clearly better than the other.

Copy link
Contributor

Choose a reason for hiding this comment

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

i wrote it up quick at #1026, take a look

Copy link
Contributor

@balopat balopat Oct 1, 2018

Choose a reason for hiding this comment

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

My 2 cents:
The two solution seem pretty comparable, but I like the blang/semver version more because it seems like that the K8s versioning is a subset of semver, hence the abstraction is correct.

The lib will take care of version ordering after the mapping - and for the other blang/semver functionality - like range parsing - will be available, only we'd have to do similar mapping but could be useful and then we don't have to write tests for our own ordering.

One issue for example with Dan's approach already is that "v1" is not supported. "ga" is not following the K8S versioning either. By mapping <alpha/beta>X to Semver's pre-release numbers, this goes away automatically.

One problem I see with our approach is that there is no minor version numbers - but we made that call when we went with the K8S versioning.

res := re.FindStringSubmatch(v)
if len(res) == 0 {
return nil, fmt.Errorf("%s is an invalid api version", v)
}

major, err := strconv.Atoi(res[1])
if err != nil {
return nil, fmt.Errorf("%s is an invalid major version number", res[1])
}

track := ga
switch res[2] {
case "alpha":
track = alpha
case "beta":
track = beta
}

av := Version{
Major: major,
Release: track,
}

if track != ga {
minor, err := strconv.Atoi(res[3])
if err != nil {
return nil, fmt.Errorf("%s is an invalid major version number", res[1])
}
av.Minor = minor
}
return &av, nil
}

// MustParseVersion parses the version and panics if there is an error.
func MustParseVersion(v string) *Version {
av, err := ParseVersion(v)
if err != nil {
panic(err)
}
return av
}

// Compare compares the Version to another Version, and returns -1 if v is less than ov, 0 if they are equal and 1 if v is greater than ov.
func (v *Version) Compare(ov *Version) int {
// GA is always higher than beta, beta is always higher than alpha
if v.Release != ov.Release {
if v.Release > ov.Release {
return 1
}
return -1
}

// v2alpha > v1beta
if v.Major != ov.Major {
if v.Major > ov.Major {
return 1
}
return -1
}

if v.Minor > ov.Minor {
return 1
}
if v.Minor < ov.Minor {
return -1
}
return 0
}

// LT returns true if v is less than ov
func (v *Version) LT(ov *Version) bool {
return v.Compare(ov) == -1
}

// GT returns true if v is greather than ov
func (v *Version) GT(ov *Version) bool {
return v.Compare(ov) == 1
}
Loading