From a1e29ae290870e05b5aa12b8731c4b89c839646c Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 19 Apr 2017 17:04:09 -0700 Subject: [PATCH] plugin/discovery: use go-version instead of semver The semver library we were using doesn't have support for a "pessimistic constraint" where e.g. the user wants to accept only minor or patch version upgrades. This is important for providers since users will generally want to pin their dependencies to not inadvertantly accept breaking changes. So here we switch to hashicorp's home-grown go-version library, which has the ~> constraint operator for this sort of constraint. Given how much the old version object was already intruding into the interface and creating dependency noise in callers, this also now wraps the "raw" go-version objects in package-local structs, thus keeping the details encapsulated and allowing callers to deal just with this package's own types. --- plugin/discovery/find.go | 2 +- plugin/discovery/meta.go | 13 +----- plugin/discovery/meta_set.go | 24 +++++------ plugin/discovery/meta_set_test.go | 16 +++---- plugin/discovery/requirements.go | 26 +++++++++++ plugin/discovery/version.go | 37 ++++++++++++++++ plugin/discovery/version_set.go | 64 ++++++++++++++++++++++++++++ plugin/discovery/version_set_test.go | 64 ++++++++++++++++++++++++++++ 8 files changed, 211 insertions(+), 35 deletions(-) create mode 100644 plugin/discovery/requirements.go create mode 100644 plugin/discovery/version.go create mode 100644 plugin/discovery/version_set.go create mode 100644 plugin/discovery/version_set_test.go diff --git a/plugin/discovery/find.go b/plugin/discovery/find.go index 603a49118..923923dcc 100644 --- a/plugin/discovery/find.go +++ b/plugin/discovery/find.go @@ -157,7 +157,7 @@ func ResolvePluginPaths(paths []string) PluginMetaSet { s.Add(PluginMeta{ Name: name, - Version: version, + Version: VersionStr(version), Path: path, }) found[nameVersion{name, version}] = struct{}{} diff --git a/plugin/discovery/meta.go b/plugin/discovery/meta.go index d93296cfc..bdcebcb9d 100644 --- a/plugin/discovery/meta.go +++ b/plugin/discovery/meta.go @@ -4,8 +4,6 @@ import ( "crypto/sha256" "io" "os" - - "github.com/blang/semver" ) // PluginMeta is metadata about a plugin, useful for launching the plugin @@ -16,21 +14,14 @@ type PluginMeta struct { Name string // Version is the semver version of the plugin, expressed as a string - // that might not be semver-valid. (Call VersionObj to attempt to - // parse it and thus detect if it is invalid.) - Version string + // that might not be semver-valid. + Version VersionStr // Path is the absolute path of the executable that can be launched // to provide the RPC server for this plugin. Path string } -// VersionObj returns the semver version of the plugin as an object, or -// an error if the version string is not semver-syntax-compliant. -func (m PluginMeta) VersionObj() (semver.Version, error) { - return semver.Make(m.Version) -} - // SHA256 returns a SHA256 hash of the content of the referenced executable // file, or an error if the file's contents cannot be read. func (m PluginMeta) SHA256() ([]byte, error) { diff --git a/plugin/discovery/meta_set.go b/plugin/discovery/meta_set.go index 37220f515..066870e6e 100644 --- a/plugin/discovery/meta_set.go +++ b/plugin/discovery/meta_set.go @@ -1,9 +1,5 @@ package discovery -import ( - "github.com/blang/semver" -) - // A PluginMetaSet is a set of PluginMeta objects meeting a certain criteria. // // Methods on this type allow filtering of the set to produce subsets that @@ -44,7 +40,7 @@ func (s PluginMetaSet) ValidateVersions() (valid, invalid PluginMetaSet) { valid = make(PluginMetaSet) invalid = make(PluginMetaSet) for p := range s { - if _, err := p.VersionObj(); err == nil { + if _, err := p.Version.Parse(); err == nil { valid.Add(p) } else { invalid.Add(p) @@ -97,14 +93,14 @@ func (s PluginMetaSet) Newest() PluginMeta { var first = true var winner PluginMeta - var winnerVersion semver.Version + var winnerVersion Version for p := range s { - version, err := p.VersionObj() + version, err := p.Version.Parse() if err != nil { panic(err) } - if first == true || version.GT(winnerVersion) { + if first == true || version.newerThan(winnerVersion) { winner = p winnerVersion = version first = false @@ -114,11 +110,11 @@ func (s PluginMetaSet) Newest() PluginMeta { return winner } -// ConstrainVersions takes a map of version constraints by name and attempts to +// ConstrainVersions takes a set of requirements and attempts to // return a map from name to a set of metas that have the matching // name and an appropriate version. // -// If any of the given constraints match *no* plugins then its PluginMetaSet +// If any of the given requirements match *no* plugins then its PluginMetaSet // in the returned map will be nil. // // All viable metas are returned, so the caller can apply any desired filtering @@ -128,22 +124,22 @@ func (s PluginMetaSet) Newest() PluginMeta { // If any of the metas in the set have invalid version strings then this // function will panic. Use ValidateVersions() first to filter out metas with // invalid versions. -func (s PluginMetaSet) ConstrainVersions(reqd map[string]semver.Range) map[string]PluginMetaSet { +func (s PluginMetaSet) ConstrainVersions(reqd PluginRequirements) map[string]PluginMetaSet { ret := make(map[string]PluginMetaSet) for p := range s { name := p.Name - constraint, ok := reqd[name] + allowedVersions, ok := reqd[name] if !ok { continue } if _, ok := ret[p.Name]; !ok { ret[p.Name] = make(PluginMetaSet) } - version, err := p.VersionObj() + version, err := p.Version.Parse() if err != nil { panic(err) } - if constraint(version) { + if allowedVersions.Has(version) { ret[p.Name].Add(p) } } diff --git a/plugin/discovery/meta_set_test.go b/plugin/discovery/meta_set_test.go index 21aaed29d..18485f58d 100644 --- a/plugin/discovery/meta_set_test.go +++ b/plugin/discovery/meta_set_test.go @@ -4,8 +4,6 @@ import ( "fmt" "strings" "testing" - - "github.com/blang/semver" ) func TestPluginMetaSetManipulation(t *testing.T) { @@ -264,13 +262,13 @@ func TestPluginMetaSetNewest(t *testing.T) { for _, version := range test.versions { s.Add(PluginMeta{ Name: "foo", - Version: version, + Version: VersionStr(version), Path: "foo-V" + version, }) } newest := s.Newest() - if newest.Version != test.want { + if newest.Version != VersionStr(test.want) { t.Errorf("version is %q; want %q", newest.Version, test.want) } }) @@ -311,11 +309,11 @@ func TestPluginMetaSetConstrainVersions(t *testing.T) { s.Add(p) } - byName := s.ConstrainVersions(map[string]semver.Range{ - "foo": semver.MustParseRange(">=2.0.0"), - "bar": semver.MustParseRange(">=0.0.0"), - "baz": semver.MustParseRange(">=1.0.0"), - "fun": semver.MustParseRange(">5.0.0"), + byName := s.ConstrainVersions(PluginRequirements{ + "foo": ConstraintStr(">=2.0.0").MustParse(), + "bar": ConstraintStr(">=0.0.0").MustParse(), + "baz": ConstraintStr(">=1.0.0").MustParse(), + "fun": ConstraintStr(">5.0.0").MustParse(), }) if got, want := len(byName), 3; got != want { t.Errorf("%d keys in map; want %d", got, want) diff --git a/plugin/discovery/requirements.go b/plugin/discovery/requirements.go new file mode 100644 index 000000000..e82909540 --- /dev/null +++ b/plugin/discovery/requirements.go @@ -0,0 +1,26 @@ +package discovery + +// PluginRequirements describes a set of plugins (assumed to be of a consistent +// kind) that are required to exist and have versions within the given +// corresponding sets. +// +// PluginRequirements is a map from plugin name to VersionSet. +type PluginRequirements map[string]VersionSet + +// Merge takes the contents of the receiver and the other given requirements +// object and merges them together into a single requirements structure +// that satisfies both sets of requirements. +func (r PluginRequirements) Merge(other PluginRequirements) PluginRequirements { + ret := make(PluginRequirements) + for n, vs := range r { + ret[n] = vs + } + for n, vs := range other { + if existing, exists := ret[n]; exists { + ret[n] = existing.Intersection(vs) + } else { + ret[n] = vs + } + } + return ret +} diff --git a/plugin/discovery/version.go b/plugin/discovery/version.go new file mode 100644 index 000000000..160a969b0 --- /dev/null +++ b/plugin/discovery/version.go @@ -0,0 +1,37 @@ +package discovery + +import ( + version "github.com/hashicorp/go-version" +) + +// A VersionStr is a string containing a possibly-invalid representation +// of a semver version number. Call Parse on it to obtain a real Version +// object, or discover that it is invalid. +type VersionStr string + +// Parse transforms a VersionStr into a Version if it is +// syntactically valid. If it isn't then an error is returned instead. +func (s VersionStr) Parse() (Version, error) { + raw, err := version.NewVersion(string(s)) + if err != nil { + return Version{}, err + } + return Version{raw}, nil +} + +// Version represents a version number that has been parsed from +// a semver string and known to be valid. +type Version struct { + // We wrap this here just because it avoids a proliferation of + // direct go-version imports all over the place, and keeps the + // version-processing details within this package. + raw *version.Version +} + +func (v Version) String() string { + return v.raw.String() +} + +func (v Version) newerThan(other Version) bool { + return v.raw.GreaterThan(other.raw) +} diff --git a/plugin/discovery/version_set.go b/plugin/discovery/version_set.go new file mode 100644 index 000000000..951000b20 --- /dev/null +++ b/plugin/discovery/version_set.go @@ -0,0 +1,64 @@ +package discovery + +import ( + version "github.com/hashicorp/go-version" +) + +// A ConstraintStr is a string containing a possibly-invalid representation +// of a version constraint provided in configuration. Call Parse on it to +// obtain a real Constraint object, or discover that it is invalid. +type ConstraintStr string + +// Parse transforms a ConstraintStr into a VersionSet if it is +// syntactically valid. If it isn't then an error is returned instead. +func (s ConstraintStr) Parse() (VersionSet, error) { + raw, err := version.NewConstraint(string(s)) + if err != nil { + return VersionSet{}, err + } + return VersionSet{raw}, nil +} + +// MustParse is like Parse but it panics if the constraint string is invalid. +func (s ConstraintStr) MustParse() VersionSet { + ret, err := s.Parse() + if err != nil { + panic(err) + } + return ret +} + +// VersionSet represents a set of versions which any given Version is either +// a member of or not. +type VersionSet struct { + // Internally a version set is actually a list of constraints that + // *remove* versions from the set. Thus a VersionSet with an empty + // Constraints list would be one that contains *all* versions. + raw version.Constraints +} + +// Has returns true if the given version is in the receiving set. +func (s VersionSet) Has(v Version) bool { + return s.raw.Check(v.raw) +} + +// Intersection combines the receving set with the given other set to produce a +// set that is the intersection of both sets, which is to say that it contains +// only the versions that are members of both sets. +func (s VersionSet) Intersection(other VersionSet) VersionSet { + raw := make(version.Constraints, 0, len(s.raw)+len(other.raw)) + + // Since "raw" is a list of constraints that remove versions from the set, + // "Intersection" is implemented by concatenating together those lists, + // thus leaving behind only the versions not removed by either list. + raw = append(raw, s.raw...) + raw = append(raw, other.raw...) + + return VersionSet{raw} +} + +// String returns a string representation of the set members as a set +// of range constraints. +func (s VersionSet) String() string { + return s.raw.String() +} diff --git a/plugin/discovery/version_set_test.go b/plugin/discovery/version_set_test.go new file mode 100644 index 000000000..ecd3b12d0 --- /dev/null +++ b/plugin/discovery/version_set_test.go @@ -0,0 +1,64 @@ +package discovery + +import ( + "fmt" + "testing" +) + +func TestVersionSet(t *testing.T) { + tests := []struct { + ConstraintStr string + VersionStr string + ShouldHave bool + }{ + // These test cases are not exhaustive since the underlying go-version + // library is well-tested. This is mainly here just to exercise our + // wrapper code, but also used as an opportunity to cover some basic + // but important cases such as the ~> constraint so that we'll be more + // likely to catch any accidental breaking behavior changes in the + // underlying library. + { + ">=1.0.0", + "1.0.0", + true, + }, + { + ">=1.0.0", + "0.0.0", + false, + }, + { + ">=1.0.0", + "1.1.0-beta1", + true, + }, + { + "~>1.1.0", + "1.1.2-beta1", + true, + }, + { + "~>1.1.0", + "1.2.0", + false, + }, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("%s has %s", test.ConstraintStr, test.VersionStr), func(t *testing.T) { + accepted, err := ConstraintStr(test.ConstraintStr).Parse() + if err != nil { + t.Fatalf("unwanted error parsing constraints string %q: %s", test.ConstraintStr, err) + } + + version, err := VersionStr(test.VersionStr).Parse() + if err != nil { + t.Fatalf("unwanted error parsing version string %q: %s", test.VersionStr, err) + } + + if got, want := accepted.Has(version), test.ShouldHave; got != want { + t.Errorf("Has returned %#v; want %#v", got, want) + } + }) + } +}