Skip to content

Commit

Permalink
Plugin versions are now considered single numbers with optional "v" p…
Browse files Browse the repository at this point in the history 
…refix

and stability "alpha" or "beta" suffix, ex. "v2-alpha". Minor and patch
version changes are poorly defined and may constitute breaking changes;
having a single version number makes versioning simpler and permits breaking
changes to be grouped into one "major" release for a plugin.

*: remove references to minor and patch versions for plugins

pkg/plugin: implement a Version type returned by Base.Version() that is
easily parsed and validated

pkg/cli: simplify plugin resolver given the minor/patch number drop
  • Loading branch information
@estroz
estroz committed Jun 4, 2020
1 parent 1972f4c commit e689089
Show file tree
Hide file tree
Showing 13 changed files with 438 additions and 321 deletions.
44 changes: 3 additions & 41 deletions pkg/cli/cli.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ func (c *cli) initialize() error {
// In case 1, default plugins will be used to determine which plugin to use.
// In case 2, the value passed to --plugins is used.
// For all other commands, a config's 'layout' key is used. Since both
// layout and --plugins values can be short (ex. "go/v2.0.0") or unversioned
// layout and --plugins values can be short (ex. "go/v2") or unversioned
// (ex. "go.kubebuilder.io") keys or both, their values may need to be
// resolved to known plugins by key.
// Default plugins are checked first so any input key that has more than one
Expand Down Expand Up @@ -323,53 +323,15 @@ func (c cli) validate() error {
}
// CLI-set plugins do not have to contain a version.
if pluginVersion != "" {
if err := plugin.ValidateVersion(pluginVersion); err != nil {
return fmt.Errorf("invalid plugin %q version %q: %v",
pluginName, pluginVersion, err)
if _, err := plugin.ParseVersion(pluginVersion); err != nil {
return fmt.Errorf("invalid plugin version %q: %v", pluginVersion, err)
}
}
}

return nil
}

// validatePlugins validates the name and versions of a list of plugins.
func validatePlugins(plugins ...plugin.Base) error {
pluginKeySet := make(map[string]struct{}, len(plugins))
for _, p := range plugins {
if err := validatePlugin(p); err != nil {
return err
}
// Check for duplicate plugin keys.
pluginKey := plugin.KeyFor(p)
if _, seen := pluginKeySet[pluginKey]; seen {
return fmt.Errorf("two plugins have the same key: %q", pluginKey)
}
pluginKeySet[pluginKey] = struct{}{}
}
return nil
}

// validatePlugin validates the name and versions of a plugin.
func validatePlugin(p plugin.Base) error {
pluginName := p.Name()
if err := plugin.ValidateName(pluginName); err != nil {
return fmt.Errorf("invalid plugin name %q: %v", pluginName, err)
}
pluginVersion := p.Version()
if err := plugin.ValidateVersion(pluginVersion); err != nil {
return fmt.Errorf("invalid plugin %q version %q: %v",
pluginName, pluginVersion, err)
}
for _, projectVersion := range p.SupportedProjectVersions() {
if err := validation.ValidateProjectVersion(projectVersion); err != nil {
return fmt.Errorf("invalid plugin %q supported project version %q: %v",
pluginName, projectVersion, err)
}
}
return nil
}

// buildRootCmd returns a root command with a subcommand tree reflecting the
// current project's state.
func (c cli) buildRootCmd() *cobra.Command {
Expand Down
11 changes: 8 additions & 3 deletions pkg/cli/cli_suite_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,13 @@ func TestCLI(t *testing.T) {

// Test plugin types and constructors.
type mockPlugin struct {
name, version string
name string
version plugin.Version
projectVersions []string
}

func (p mockPlugin) Name() string { return p.name }
func (p mockPlugin) Version() string { return p.version }
func (p mockPlugin) Version() plugin.Version { return p.version }
func (p mockPlugin) SupportedProjectVersions() []string { return p.projectVersions }

func (mockPlugin) UpdateContext(*plugin.Context) {}
Expand All @@ -50,7 +51,11 @@ func (mockPlugin) InjectConfig(*config.Config) {}
func (mockPlugin) Run() error { return nil }

func makeBasePlugin(name, version string, projVers ...string) plugin.Base {
return mockPlugin{name, version, projVers}
v, err := plugin.ParseVersion(version)
if err != nil {
panic(err)
}
return mockPlugin{name, v, projVers}
}

func makePluginsForKeys(keys ...string) (plugins []plugin.Base) {
Expand Down
15 changes: 9 additions & 6 deletions pkg/cli/cli_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ var _ = Describe("CLI", func() {
pluginNameA = "go.example.com"
pluginNameB = "go.test.com"
projectVersions = []string{config.Version2, config.Version3Alpha}
pluginAV1 = makeAllPlugin(pluginNameA, "v1.0", projectVersions...)
pluginAV2 = makeAllPlugin(pluginNameA, "v2.0", projectVersions...)
pluginBV1 = makeAllPlugin(pluginNameB, "v1.0", projectVersions...)
pluginBV2 = makeAllPlugin(pluginNameB, "v2.0", projectVersions...)
pluginAV1 = makeAllPlugin(pluginNameA, "v1", projectVersions...)
pluginAV2 = makeAllPlugin(pluginNameA, "v2", projectVersions...)
pluginBV1 = makeAllPlugin(pluginNameB, "v1", projectVersions...)
pluginBV2 = makeAllPlugin(pluginNameB, "v2", projectVersions...)
allPlugins = []plugin.Base{pluginAV1, pluginAV2, pluginBV1, pluginBV2}
)

Expand Down Expand Up @@ -89,7 +89,7 @@ var _ = Describe("CLI", func() {

By("setting two plugins of the same name and version")
_, err = New(WithDefaultPlugins(pluginAV1), WithPlugins(pluginAV1, pluginAV1))
Expect(err).To(MatchError(`broken pre-set plugins: two plugins have the same key: "go.example.com/v1.0"`))
Expect(err).To(MatchError(`broken pre-set plugins: two plugins have the same key: "go.example.com/v1"`))
})
})

Expand Down Expand Up @@ -145,7 +145,10 @@ var _ = Describe("CLI", func() {
By(`setting cliPluginKey to an non-existent key "foo"`)
setPluginsFlag("foo")
_, err = New(WithDefaultPlugins(pluginAV1), WithPlugins(pluginAV1, pluginAV2))
Expect(err).To(MatchError(errAmbiguousPlugin{"foo", "no names match"}))
Expect(err).To(MatchError(errAmbiguousPlugin{
key: "foo",
msg: `no names match, possible plugins: ["go.example.com/v1" "go.example.com/v2"]`,
}))
})
})

Expand Down
173 changes: 66 additions & 107 deletions pkg/cli/plugins.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ import (
"fmt"
"sort"

"github.com/blang/semver"

"sigs.k8s.io/kubebuilder/pkg/internal/validation"
"sigs.k8s.io/kubebuilder/pkg/plugin"
)

Expand All @@ -37,12 +36,12 @@ func (e errAmbiguousPlugin) Error() string {

// resolvePluginsByKey resolves versionedPlugins to a subset of plugins by
// matching keys to some form of pluginKey. Those forms can be a:
// - Fully qualified key: "go.kubebuilder.io/v2.0"
// - Short key: "go/v2.0"
// - Fully qualified key: "go.kubebuilder.io/v2"
// - Short key: "go/v2"
// - Fully qualified name: "go.kubebuilder.io"
// - Short name: "go"
// Some of these keys may conflict, ex. the fully-qualified and short names of
// "go.kubebuilder.io/v1.0" and "go.kubebuilder.io/v2.0" have ambiguous
// "go.kubebuilder.io/v1" and "go.kubebuilder.io/v2" have ambiguous
// unversioned names "go.kubernetes.io" and "go". If pluginKey is ambiguous
// or does not match any known plugin's key, an error is returned.
//
Expand All @@ -52,82 +51,53 @@ func resolvePluginsByKey(versionedPlugins []plugin.Base, pluginKey string) (reso

name, version := plugin.SplitKey(pluginKey)

// Compare versions first to narrow the list of name comparisons.
if version == "" {
// Case: if plugin key has no version, check all plugin names.
resolved = versionedPlugins
} else {
// Case: if plugin key has version, filter by version.
resolved = findPluginsMatchingVersion(versionedPlugins, version)
}

if len(resolved) == 0 {
return nil, errAmbiguousPlugin{pluginKey, "no versions match"}
}

// Compare names, taking into account whether name is fully-qualified or not.
shortName := plugin.GetShortName(name)
if name == shortName {
// Case: if plugin name is short, find matching short names.
resolved = findPluginsMatchingShortName(resolved, shortName)
resolved = findPluginsMatchingShortName(versionedPlugins, shortName)
} else {
// Case: if plugin name is fully-qualified, match only fully-qualified names.
resolved = findPluginsMatchingName(resolved, name)
resolved = findPluginsMatchingName(versionedPlugins, name)
}

if len(resolved) == 0 {
return nil, errAmbiguousPlugin{pluginKey, "no names match"}
}

// Since plugins has already been resolved by matching names and versions,
// it should only contain one matching value for a versionless pluginKey if
// it isn't ambiguous.
if version == "" {
if len(resolved) == 1 {
return resolved, nil
return nil, errAmbiguousPlugin{
key: pluginKey,
msg: fmt.Sprintf("no names match, possible plugins: %+q", makePluginKeySlice(versionedPlugins...)),
}
return nil, errAmbiguousPlugin{pluginKey, fmt.Sprintf("possible keys: %+q", makePluginKeySlice(resolved...))}
}

rp, err := resolveToPlugin(resolved)
if err != nil {
return nil, errAmbiguousPlugin{pluginKey, err.Error()}
}
return []plugin.Base{rp}, nil
}

// findPluginsMatchingVersion returns a set of plugins with Version() matching
// version. The set will contain plugins with either major and minor versions
// matching exactly or major versions matching exactly and greater minor versions,
// but not a mix of the two match types.
func findPluginsMatchingVersion(plugins []plugin.Base, version string) []plugin.Base {
// Assume versions have been validated already.
v := must(semver.ParseTolerant(version))

var equal, matchingMajor []plugin.Base
for _, p := range plugins {
pv := must(semver.ParseTolerant(p.Version()))
if v.Major == pv.Major {
if v.Minor == pv.Minor {
equal = append(equal, p)
} else if v.Minor < pv.Minor {
matchingMajor = append(matchingMajor, p)
if version != "" {
// Case: if plugin key has version, filter by version.
v, err := plugin.ParseVersion(version)
if err != nil {
return nil, err
}
keys := makePluginKeySlice(resolved...)
for i := 0; i < len(resolved); i++ {
if v.Compare(resolved[i].Version()) != 0 {
resolved = append(resolved[:i], resolved[i+1:]...)
i--
}
}
if len(resolved) == 0 {
return nil, errAmbiguousPlugin{
key: pluginKey,
msg: fmt.Sprintf("no versions match, possible plugins: %+q", keys),
}
}
}

if len(equal) != 0 {
return equal
}
return matchingMajor
}

// must wraps semver.Parse and panics if err is non-nil.
func must(v semver.Version, err error) semver.Version {
if err != nil {
panic(err)
// Since plugins has already been resolved by matching names and versions,
// it should only contain one matching value if it isn't ambiguous.
if len(resolved) != 1 {
return nil, errAmbiguousPlugin{
key: pluginKey,
msg: fmt.Sprintf("matching plugins: %+q", makePluginKeySlice(resolved...)),
}
}
return v
return resolved, nil
}

// findPluginsMatchingName returns a set of plugins with Name() exactly
Expand All @@ -152,56 +122,45 @@ func findPluginsMatchingShortName(plugins []plugin.Base, shortName string) (equa
return equal
}

// resolveToPlugin returns a single plugin from plugins given the following
// conditions about plugins:
// 1. len(plugins) > 0.
// 2. No two plugin names are different.
// An error is returned if either condition is invalidated.
func resolveToPlugin(plugins []plugin.Base) (rp plugin.Base, err error) {
// Versions are either an exact match or have greater minor versions, so
// we choose the last in a sorted list of versions to get the correct one.
versions := make([]semver.Version, len(plugins))
for i, p := range plugins {
versions[i] = must(semver.ParseTolerant(p.Version()))
}
if len(versions) == 0 {
return nil, fmt.Errorf("possible versions: %+q", versions)
// makePluginKeySlice returns a slice of all keys for each plugin in plugins.
func makePluginKeySlice(plugins ...plugin.Base) (keys []string) {
for _, p := range plugins {
keys = append(keys, plugin.KeyFor(p))
}
semver.Sort(versions)
useVersion := versions[len(versions)-1]
sort.Strings(keys)
return
}

// If more than one name in plugins exists, the name portion of pluginKey
// needs to be more specific.
nameSet := make(map[string]struct{})
// validatePlugins validates the name and versions of a list of plugins.
func validatePlugins(plugins ...plugin.Base) error {
pluginKeySet := make(map[string]struct{}, len(plugins))
for _, p := range plugins {
nameSet[p.Name()] = struct{}{}
// This condition will only be true once for an unambiguous plugin name,
// since plugin keys have been checked for duplicates already.
if must(semver.ParseTolerant(p.Version())).Equals(useVersion) {
rp = p
if err := validatePlugin(p); err != nil {
return err
}
// Check for duplicate plugin keys.
pluginKey := plugin.KeyFor(p)
if _, seen := pluginKeySet[pluginKey]; seen {
return fmt.Errorf("two plugins have the same key: %q", pluginKey)
}
pluginKeySet[pluginKey] = struct{}{}
}
if len(nameSet) != 1 {
return nil, fmt.Errorf("possible names: %+q", makeKeySlice(nameSet))
}

return rp, nil
return nil
}

// makeKeySlice returns a slice of all map keys in set.
func makeKeySlice(set map[string]struct{}) (keys []string) {
for key := range set {
keys = append(keys, key)
// validatePlugin validates the name and versions of a plugin.
func validatePlugin(p plugin.Base) error {
pluginName := p.Name()
if err := plugin.ValidateName(pluginName); err != nil {
return fmt.Errorf("invalid plugin name %q: %v", pluginName, err)
}
sort.Strings(keys)
return
}

// makePluginKeySlice returns a slice of all keys for each plugin in plugins.
func makePluginKeySlice(plugins ...plugin.Base) (keys []string) {
for _, p := range plugins {
keys = append(keys, plugin.KeyFor(p))
if err := p.Version().Validate(); err != nil {
return fmt.Errorf("invalid plugin version %q: %v", p.Version(), err)
}
sort.Strings(keys)
return
for _, projectVersion := range p.SupportedProjectVersions() {
if err := validation.ValidateProjectVersion(projectVersion); err != nil {
return fmt.Errorf("invalid project version %q: %v", projectVersion, err)
}
}
return nil
}

0 comments on commit e689089

Please sign in to comment.