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

cli-plugins: add concept of experimental plugin, only enabled in experimental mode #1898

Merged
merged 2 commits into from
May 23, 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cli-plugins/examples/helloworld/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,6 @@ func main() {
SchemaVersion: "0.1.0",
Vendor: "Docker Inc.",
Version: "testing",
Experimental: os.Getenv("HELLO_EXPERIMENTAL") != "",
})
}
72 changes: 40 additions & 32 deletions cli-plugins/manager/candidate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@ import (
)

type fakeCandidate struct {
path string
exec bool
meta string
path string
exec bool
meta string
allowExperimental bool
}

func (c *fakeCandidate) Path() string {
Expand All @@ -35,9 +36,10 @@ func TestValidateCandidate(t *testing.T) {
builtinName = NamePrefix + "builtin"
builtinAlias = NamePrefix + "alias"

badPrefixPath = "/usr/local/libexec/cli-plugins/wobble"
badNamePath = "/usr/local/libexec/cli-plugins/docker-123456"
goodPluginPath = "/usr/local/libexec/cli-plugins/" + goodPluginName
badPrefixPath = "/usr/local/libexec/cli-plugins/wobble"
badNamePath = "/usr/local/libexec/cli-plugins/docker-123456"
goodPluginPath = "/usr/local/libexec/cli-plugins/" + goodPluginName
metaExperimental = `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing", "Experimental": true}`
)

fakeroot := &cobra.Command{Use: "docker"}
Expand All @@ -49,40 +51,46 @@ func TestValidateCandidate(t *testing.T) {
})

for _, tc := range []struct {
c *fakeCandidate
name string
c *fakeCandidate

// Either err or invalid may be non-empty, but not both (both can be empty for a good plugin).
err string
invalid string
}{
/* Each failing one of the tests */
{c: &fakeCandidate{path: ""}, err: "plugin candidate path cannot be empty"},
{c: &fakeCandidate{path: badPrefixPath}, err: fmt.Sprintf("does not have %q prefix", NamePrefix)},
{c: &fakeCandidate{path: badNamePath}, invalid: "did not match"},
{c: &fakeCandidate{path: builtinName}, invalid: `plugin "builtin" duplicates builtin command`},
{c: &fakeCandidate{path: builtinAlias}, invalid: `plugin "alias" duplicates an alias of builtin command "builtin"`},
{c: &fakeCandidate{path: goodPluginPath, exec: false}, invalid: fmt.Sprintf("failed to fetch metadata: faked a failure to exec %q", goodPluginPath)},
{c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `xyzzy`}, invalid: "invalid character"},
{c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{}`}, invalid: `plugin SchemaVersion "" is not valid`},
{c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "xyzzy"}`}, invalid: `plugin SchemaVersion "xyzzy" is not valid`},
{c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0"}`}, invalid: "plugin metadata does not define a vendor"},
{c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": ""}`}, invalid: "plugin metadata does not define a vendor"},
{name: "empty path", c: &fakeCandidate{path: ""}, err: "plugin candidate path cannot be empty"},
{name: "bad prefix", c: &fakeCandidate{path: badPrefixPath}, err: fmt.Sprintf("does not have %q prefix", NamePrefix)},
{name: "bad path", c: &fakeCandidate{path: badNamePath}, invalid: "did not match"},
{name: "builtin command", c: &fakeCandidate{path: builtinName}, invalid: `plugin "builtin" duplicates builtin command`},
{name: "builtin alias", c: &fakeCandidate{path: builtinAlias}, invalid: `plugin "alias" duplicates an alias of builtin command "builtin"`},
{name: "fetch failure", c: &fakeCandidate{path: goodPluginPath, exec: false}, invalid: fmt.Sprintf("failed to fetch metadata: faked a failure to exec %q", goodPluginPath)},
{name: "metadata not json", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `xyzzy`}, invalid: "invalid character"},
{name: "empty schemaversion", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{}`}, invalid: `plugin SchemaVersion "" is not valid`},
{name: "invalid schemaversion", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "xyzzy"}`}, invalid: `plugin SchemaVersion "xyzzy" is not valid`},
{name: "no vendor", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0"}`}, invalid: "plugin metadata does not define a vendor"},
{name: "empty vendor", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": ""}`}, invalid: "plugin metadata does not define a vendor"},
{name: "experimental required", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: metaExperimental}, invalid: "requires experimental CLI"},
// This one should work
{c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing"}`}},
{name: "valid", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing"}`}},
{name: "valid + allowing experimental", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: `{"SchemaVersion": "0.1.0", "Vendor": "e2e-testing"}`, allowExperimental: true}},
{name: "experimental + allowing experimental", c: &fakeCandidate{path: goodPluginPath, exec: true, meta: metaExperimental, allowExperimental: true}},
} {
p, err := newPlugin(tc.c, fakeroot)
if tc.err != "" {
assert.ErrorContains(t, err, tc.err)
} else if tc.invalid != "" {
assert.NilError(t, err)
assert.Assert(t, cmp.ErrorType(p.Err, reflect.TypeOf(&pluginError{})))
assert.ErrorContains(t, p.Err, tc.invalid)
} else {
assert.NilError(t, err)
assert.Equal(t, NamePrefix+p.Name, goodPluginName)
assert.Equal(t, p.SchemaVersion, "0.1.0")
assert.Equal(t, p.Vendor, "e2e-testing")
}
t.Run(tc.name, func(t *testing.T) {
p, err := newPlugin(tc.c, fakeroot, tc.c.allowExperimental)
if tc.err != "" {
assert.ErrorContains(t, err, tc.err)
} else if tc.invalid != "" {
assert.NilError(t, err)
assert.Assert(t, cmp.ErrorType(p.Err, reflect.TypeOf(&pluginError{})))
assert.ErrorContains(t, p.Err, tc.invalid)
} else {
assert.NilError(t, err)
assert.Equal(t, NamePrefix+p.Name, goodPluginName)
assert.Equal(t, p.SchemaVersion, "0.1.0")
assert.Equal(t, p.Vendor, "e2e-testing")
}
})
}
}

Expand Down
31 changes: 27 additions & 4 deletions cli-plugins/manager/manager.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package manager

import (
"fmt"
"io/ioutil"
"os"
"os/exec"
Expand All @@ -27,10 +28,23 @@ func (e errPluginNotFound) Error() string {
return "Error: No such CLI plugin: " + string(e)
}

type errPluginRequireExperimental string

// Note: errPluginRequireExperimental implements notFound so that the plugin
// is skipped when listing the plugins.
func (e errPluginRequireExperimental) NotFound() {}

func (e errPluginRequireExperimental) Error() string {
return fmt.Sprintf("plugin candidate %q: requires experimental CLI", string(e))
}

type notFound interface{ NotFound() }

// IsNotFound is true if the given error is due to a plugin not being found.
func IsNotFound(err error) bool {
if e, ok := err.(*pluginError); ok {
err = e.Cause()
}
_, ok := err.(notFound)
return ok
}
Expand Down Expand Up @@ -117,12 +131,14 @@ func ListPlugins(dockerCli command.Cli, rootcmd *cobra.Command) ([]Plugin, error
continue
}
c := &candidate{paths[0]}
p, err := newPlugin(c, rootcmd)
p, err := newPlugin(c, rootcmd, dockerCli.ClientInfo().HasExperimental)
if err != nil {
return nil, err
}
p.ShadowedPaths = paths[1:]
plugins = append(plugins, p)
if !IsNotFound(p.Err) {
p.ShadowedPaths = paths[1:]
plugins = append(plugins, p)
}
}

return plugins, nil
Expand Down Expand Up @@ -159,12 +175,19 @@ func PluginRunCommand(dockerCli command.Cli, name string, rootcmd *cobra.Command
}

c := &candidate{path: path}
plugin, err := newPlugin(c, rootcmd)
plugin, err := newPlugin(c, rootcmd, dockerCli.ClientInfo().HasExperimental)
if err != nil {
return nil, err
}
if plugin.Err != nil {
// TODO: why are we not returning plugin.Err?

err := plugin.Err.(*pluginError).Cause()
// if an experimental plugin was invoked directly while experimental mode is off
// provide a more useful error message than "not found".
if err, ok := err.(errPluginRequireExperimental); ok {
return nil, err
}
return nil, errPluginNotFound(name)
}
cmd := exec.Command(plugin.Path, args...)
Expand Down
3 changes: 3 additions & 0 deletions cli-plugins/manager/metadata.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,7 @@ type Metadata struct {
ShortDescription string `json:",omitempty"`
// URL is a pointer to the plugin's homepage.
URL string `json:",omitempty"`
// Experimental specifies whether the plugin is experimental.
// Experimental plugins are not displayed on non-experimental CLIs.
Experimental bool `json:",omitempty"`
}
9 changes: 7 additions & 2 deletions cli-plugins/manager/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ type Plugin struct {
// is set, and is always a `pluginError`, but the `Plugin` is still
// returned with no error. An error is only returned due to a
// non-recoverable error.
func newPlugin(c Candidate, rootcmd *cobra.Command) (Plugin, error) {
//
// nolint: gocyclo
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be done in a follow-up, but I think this function should be split into pieces as it reached the go-cyclo threshold.

func newPlugin(c Candidate, rootcmd *cobra.Command, allowExperimental bool) (Plugin, error) {
path := c.Path()
if path == "" {
return Plugin{}, errors.New("plugin candidate path cannot be empty")
Expand Down Expand Up @@ -94,7 +96,10 @@ func newPlugin(c Candidate, rootcmd *cobra.Command) (Plugin, error) {
p.Err = wrapAsPluginError(err, "invalid metadata")
return p, nil
}

if p.Experimental && !allowExperimental {
p.Err = &pluginError{errPluginRequireExperimental(p.Name)}
return p, nil
}
if p.Metadata.SchemaVersion != "0.1.0" {
p.Err = NewPluginError("plugin SchemaVersion %q is not valid, must be 0.1.0", p.Metadata.SchemaVersion)
return p, nil
Expand Down