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

fix: improve plugin error message #371

Merged
merged 20 commits into from
Jan 18, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
32 changes: 4 additions & 28 deletions plugin/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import "errors"
var ErrNotCompliant = errors.New("plugin not compliant")

// ErrNotRegularFile is returned when the plugin file is not an regular file.
var ErrNotRegularFile = errors.New("not regular file")
var ErrNotRegularFile = errors.New("plugin executable file is not regular file")
JeyJeyGao marked this conversation as resolved.
Show resolved Hide resolved

// PluginDowngradeError is returned when installing a plugin with version
// lower than the exisiting plugin version.
Expand Down Expand Up @@ -92,33 +92,9 @@ func (e PluginMalformedError) Unwrap() error {

// PluginDirectryWalkError is used when there is an issue with plugins directory
// and should suggest user to check the permission of plugin directory.
type PluginDirectryWalkError struct {
Err error
}

// Error returns the error message.
func (e PluginDirectryWalkError) Error() string {
return e.Err.Error()
}

// Unwrap returns the inner error.
func (e PluginDirectryWalkError) Unwrap() error {
return errors.Unwrap(e.Err)
}
type PluginDirectryWalkError error
JeyJeyGao marked this conversation as resolved.
Show resolved Hide resolved
JeyJeyGao marked this conversation as resolved.
Show resolved Hide resolved

// PluginExectableFileError is used when there is an issue with plugin
// PluginExecutableFileError is used when there is an issue with plugin
// executable file and should suggest user to check the existence, permission
// and platform/arch compatibility of plugin.
type PluginExectableFileError struct {
Err error
}

// Error returns the error message.
func (e PluginExectableFileError) Error() string {
return e.Err.Error()
}

// Unwrap returns the inner error.
func (e PluginExectableFileError) Unwrap() error {
return errors.Unwrap(e.Err)
}
type PluginExecutableFileError error
7 changes: 6 additions & 1 deletion plugin/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,11 @@ func (m *CLIManager) Get(ctx context.Context, name string) (Plugin, error) {
// List produces a list of the plugin names on the system.
func (m *CLIManager) List(ctx context.Context) ([]string, error) {
var plugins []string
// check plugin directory exsitence
if _, err := fs.Stat(m.pluginFS, "."); errors.Is(err, os.ErrNotExist) {
JeyJeyGao marked this conversation as resolved.
Show resolved Hide resolved
return plugins, nil
JeyJeyGao marked this conversation as resolved.
Show resolved Hide resolved
}

if err := fs.WalkDir(m.pluginFS, ".", func(dir string, d fs.DirEntry, err error) error {
if err != nil {
return err
Expand All @@ -80,7 +85,7 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) {
plugins = append(plugins, d.Name())
return fs.SkipDir
}); err != nil {
return nil, &PluginDirectryWalkError{Err: fmt.Errorf("failed to list plugin: %w", err)}
return nil, PluginDirectryWalkError(fmt.Errorf("failed to list plugin: %w", err))
}
return plugins, nil
}
Expand Down
4 changes: 2 additions & 2 deletions plugin/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ func TestManager_Install(t *testing.T) {
installOpts := CLIInstallOptions{
PluginPath: newPluginFilePath,
}
expectedErrorMsg := "plugin executable name must be \"notation-foobar\" instead of \"notation-bar\""
expectedErrorMsg := "plugin executable file name must be \"notation-foobar\" instead of \"notation-bar\""
_, _, err := mgr.Install(context.Background(), installOpts)
if err == nil || err.Error() != expectedErrorMsg {
t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err)
Expand All @@ -405,7 +405,7 @@ func TestManager_Install(t *testing.T) {
installOpts := CLIInstallOptions{
PluginPath: newPluginFilePath,
}
expectedErrorMsg := "plugin executable name must be \"notation-bar\" instead of \"notation-foo\""
expectedErrorMsg := "plugin executable file name must be \"notation-bar\" instead of \"notation-foo\""
_, _, err := mgr.Install(context.Background(), installOpts)
if err == nil || err.Error() != expectedErrorMsg {
t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err)
Expand Down
25 changes: 11 additions & 14 deletions plugin/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,11 @@ func NewCLIPlugin(ctx context.Context, name, path string) (*CLIPlugin, error) {
if err != nil {
// Ignore any file which we cannot Stat
// (e.g. due to permissions or anything else).
return nil, fmt.Errorf("plugin instantiation failed because the executable file is either not found or inaccessible: %w", err)
return nil, fmt.Errorf("plugin executable file is either not found or inaccessible: %w", err)
}
if !fi.Mode().IsRegular() {
// Ignore non-regular files.
return nil, fmt.Errorf("plugin instantiation failed because the executable file %s is not a regular file", path)
return nil, ErrNotRegularFile
}

// generate plugin
Expand All @@ -106,12 +106,12 @@ func (p *CLIPlugin) GetMetadata(ctx context.Context, req *proto.GetMetadataReque
// validate metadata
if err = validate(&metadata); err != nil {
return nil, &PluginMalformedError{
Msg: fmt.Sprintf("metadata validation failed for %s plugin: %s", p.name, err),
Msg: fmt.Sprintf("metadata validation failed for plugin %s: %s", p.name, err),
InnerError: err,
}
}
if metadata.Name != p.name {
return nil, fmt.Errorf("plugin executable name must be %q instead of %q", binName(metadata.Name), filepath.Base(p.path))
return nil, fmt.Errorf("plugin executable file name must be %q instead of %q", binName(metadata.Name), filepath.Base(p.path))
}
return &metadata, nil
}
Expand Down Expand Up @@ -174,9 +174,9 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re
// serialize request
data, err := json.Marshal(req)
if err != nil {
logger.Errorf("failed to marshal request: %+v", req)
return PluginUnknownError{
Msg: fmt.Sprintf("failed to execute the %s command for %s plugin", req.Command(), pluginName),
logger.Errorf("Failed to marshal request: %+v", req)
return &PluginUnknownError{
Msg: fmt.Sprintf("failed to execute the %s command for plugin %s", req.Command(), pluginName),
InnerError: fmt.Errorf("failed to marshal request: %w", err)}
}

Expand All @@ -185,32 +185,29 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re
stdout, stderr, err := executor.Output(ctx, pluginPath, req.Command(), data)
if err != nil {
logger.Errorf("plugin %s execution status: %v", req.Command(), err)
logger.Errorf("Plugin %s returned error: %s", req.Command(), string(stderr))

if len(stderr) == 0 {
// if stderr is empty, it is possible that the plugin is not
// running properly.
return &PluginExectableFileError{
Err: fmt.Errorf("failed to execute the %s command for %s plugin: %w", req.Command(), pluginName, err),
}
return PluginExecutableFileError(fmt.Errorf("failed to execute the %s command for plugin %s: %w", req.Command(), pluginName, err))
} else {
var re proto.RequestError
jsonErr := json.Unmarshal(stderr, &re)
if jsonErr != nil {
return &PluginMalformedError{
Msg: fmt.Sprintf("failed to execute the %s command for %s plugin: the error response isn't compliant with Notation plugin requirement: %s", req.Command(), pluginName, string(stderr)),
Msg: fmt.Sprintf("failed to execute the %s command for plugin %s: the plugin isn't compliant with Notation plugin requirement: %s", req.Command(), pluginName, string(stderr)),
InnerError: jsonErr,
}
}
return fmt.Errorf("failed to execute the %s command for %s plugin: %w", req.Command(), pluginName, re)
return fmt.Errorf("failed to execute the %s command for plugin %s: %w", req.Command(), pluginName, re)
}
}

logger.Debugf("Plugin %s response: %s", req.Command(), string(stdout))
// deserialize response
if err = json.Unmarshal(stdout, resp); err != nil {
return &PluginMalformedError{
Msg: fmt.Sprintf("%s plugin response for %s command isn't compliant with Notation plugin requirement: %s", pluginName, req.Command(), string(stdout)),
Msg: fmt.Sprintf("plugin %s response for %s command isn't compliant with Notation plugin requirement: %s", pluginName, req.Command(), string(stdout)),
InnerError: err,
}
}
Expand Down
8 changes: 4 additions & 4 deletions plugin/plugin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ func TestGetMetadata(t *testing.T) {
t.Run("plugin error is in invalid json format", func(t *testing.T) {
exitErr := errors.New("unknown error")
stderr := []byte("sad")
expectedErrMsg := "failed to execute the get-plugin-metadata command for test-plugin plugin: the error response isn't compliant with Notation plugin requirement: sad"
expectedErrMsg := "failed to execute the get-plugin-metadata command for plugin test-plugin: the plugin isn't compliant with Notation plugin requirement: sad"
plugin := CLIPlugin{name: "test-plugin"}
executor = testCommander{stdout: nil, stderr: stderr, err: exitErr}
_, err := plugin.GetMetadata(context.Background(), &proto.GetMetadataRequest{})
Expand All @@ -55,7 +55,7 @@ func TestGetMetadata(t *testing.T) {
t.Run("plugin cause system error", func(t *testing.T) {
exitErr := errors.New("system error")
stderr := []byte("")
expectedErrMsg := "failed to execute the get-plugin-metadata command for test-plugin plugin: system error"
expectedErrMsg := "failed to execute the get-plugin-metadata command for plugin test-plugin: system error"
plugin := CLIPlugin{name: "test-plugin"}
executor = testCommander{stdout: nil, stderr: stderr, err: exitErr}
_, err := plugin.GetMetadata(context.Background(), &proto.GetMetadataRequest{})
Expand Down Expand Up @@ -194,7 +194,7 @@ func TestNewCLIPlugin_PathError(t *testing.T) {
})

t.Run("plugin is not a regular file", func(t *testing.T) {
expectedErrMsg := `plugin instantiation failed because the executable file ./testdata/plugins/badplugin/notation-badplugin is not a regular file`
expectedErrMsg := "plugin executable file is not regular file"
p, err := NewCLIPlugin(ctx, "badplugin", "./testdata/plugins/badplugin/notation-badplugin")
if err.Error() != expectedErrMsg {
t.Errorf("NewCLIPlugin() error = %v, want %v", err, expectedErrMsg)
Expand Down Expand Up @@ -230,7 +230,7 @@ func TestNewCLIPlugin_ValidError(t *testing.T) {
t.Run("invalid metadata name", func(t *testing.T) {
executor = testCommander{stdout: metadataJSON(invalidMetadataName)}
_, err := p.GetMetadata(ctx, &proto.GetMetadataRequest{})
if !strings.Contains(err.Error(), "executable name must be") {
if !strings.Contains(err.Error(), "executable file name must be") {
t.Fatal("should fail the operation.")
}
})
Expand Down
Loading