PR feedbackl

Signed-off-by: Pritesh Bandi <[email protected]>

cli/cli.go

@@ -10,35 +10,35 @@ package cli
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"os"
 	"reflect"
-	"strings"
 
 	"github.com/notaryproject/notation-plugin-framework-go/internal/slices"
 	"github.com/notaryproject/notation-plugin-framework-go/log"
 	"github.com/notaryproject/notation-plugin-framework-go/plugin"
 )
 
+// CLI struct is used to create an executable for plugin.
 type CLI struct {
 	name   string
 	pl     plugin.Plugin
 	logger log.Logger
 }
 
 // New creates a new CLI using given plugin
-func New(executableName string, pl plugin.Plugin) (*CLI, error) {
-	return NewWithLogger(executableName, pl, &discardLogger{})
+func New(pl plugin.Plugin) (*CLI, error) {
+	return NewWithLogger(pl, &discardLogger{})
 }
 
 // NewWithLogger creates a new CLI using given plugin and logger
-func NewWithLogger(executableName string, pl plugin.Plugin, l log.Logger) (*CLI, error) {
-	if strings.HasPrefix(executableName, plugin.BinaryPrefix) {
-		return nil, fmt.Errorf("executable name must start with prefix: %s", plugin.BinaryPrefix)
+func NewWithLogger(pl plugin.Plugin, l log.Logger) (*CLI, error) {
+	if pl == nil {
+		return nil, errors.New("plugin cannot be nil")
 	}
 
 	return &CLI{
-		name:   executableName,
 		pl:     pl,
 		logger: l,
 	}, nil
@@ -118,7 +118,7 @@ func (c *CLI) printVersion(ctx context.Context) {
 func (c *CLI) validateArgs(ctx context.Context, args []string) {
 	md := c.getMetadata(ctx, c.pl)
 	if !(len(args) == 2 && slices.Contains(getValidArgs(md), args[1])) {
-		deliverError(fmt.Sprintf("Invalid command, valid choices are: %s %s", c.name, getValidArgsString(md)))
+		deliverError(fmt.Sprintf("Invalid command, valid commands are: %s", getValidArgsString(md)))
 	}
 }
 
@@ -160,63 +160,3 @@ func (c *CLI) marshalResponse(response any, err error) (string, *plugin.Error) {
 	c.logger.Debugf("%s response: %s", reflect.TypeOf(response), jsonResponse)
 	return string(jsonResponse), nil
 }
-
-// deferStdout is used to make sure that nothing get emitted to stdout and stderr until intentionally rescued.
-// This is required to make sure that the plugin or its dependency doesn't interfere with notation <-> plugin communication
-func deferStdout() func() {
-	// Ignoring error because we don't want plugin to fail if `os.DevNull` is misconfigured.
-	null, _ := os.Open(os.DevNull)
-	sout := os.Stdout
-	serr := os.Stderr
-	os.Stdout = null
-	os.Stderr = null
-
-	return func() {
-		err := null.Close()
-		if err != nil {
-			return
-		}
-		os.Stdout = sout
-		os.Stderr = serr
-	}
-}
-
-// discardLogger implements Logger but logs nothing. It is used when user
-// disenabled logging option in notation, i.e. loggerKey is not in the context.
-type discardLogger struct{}
-
-func (dl *discardLogger) Debug(_ ...interface{}) {
-}
-
-func (dl *discardLogger) Debugf(_ string, _ ...interface{}) {
-}
-
-func (dl *discardLogger) Debugln(_ ...interface{}) {
-}
-
-func (dl *discardLogger) Info(_ ...interface{}) {
-}
-
-func (dl *discardLogger) Infof(_ string, _ ...interface{}) {
-}
-
-func (dl *discardLogger) Infoln(_ ...interface{}) {
-}
-
-func (dl *discardLogger) Warn(_ ...interface{}) {
-}
-
-func (dl *discardLogger) Warnf(_ string, _ ...interface{}) {
-}
-
-func (dl *discardLogger) Warnln(_ ...interface{}) {
-}
-
-func (dl *discardLogger) Error(_ ...interface{}) {
-}
-
-func (dl *discardLogger) Errorf(_ string, _ ...interface{}) {
-}
-
-func (dl *discardLogger) Errorln(_ ...interface{}) {
-}

cli/cli_test.go

@@ -14,7 +14,7 @@ import (
 	"github.com/notaryproject/notation-plugin-framework-go/plugin"
 )
 
-var cli = New("mockCli", &mockPlugin{})
+var cli, _ = New(&mockPlugin{})
 
 func TestMarshalResponse(t *testing.T) {
 	res := plugin.GenerateEnvelopeResponse{

cli/utils.go

@@ -17,19 +17,19 @@ func getValidArgsString(md *plugin.GetMetadataResponse) string {
 // getValidArgs returns list of valid arguments depending upon the plugin capabilities
 func getValidArgs(md *plugin.GetMetadataResponse) []string {
 	opts := []string{
-		"get-plugin-metadata", "version",
+		string(plugin.CommandGetMetadata), "version",
 	}
 
 	if slices.Contains(md.Capabilities, plugin.CapabilitySignatureGenerator) {
-		opts = append(opts, "generate-signature", "describe-key")
+		opts = append(opts, string(plugin.CommandGenerateSignature), string(plugin.CommandDescribeKey))
 	}
 
 	if slices.Contains(md.Capabilities, plugin.CapabilityEnvelopeGenerator) {
-		opts = append(opts, "generate-envelope")
+		opts = append(opts, string(plugin.CommandGenerateEnvelope))
 	}
 
 	if slices.Contains(md.Capabilities, plugin.CapabilityTrustedIdentityVerifier) || slices.Contains(md.Capabilities, plugin.CapabilityRevocationCheckVerifier) {
-		opts = append(opts, "verify-signature")
+		opts = append(opts, string(plugin.CommandVerifySignature))
 	}
 	sort.Strings(opts)
 	return opts
@@ -40,3 +40,63 @@ func deliverError(message string) {
 	_, _ = fmt.Fprintln(os.Stderr, message)
 	os.Exit(1)
 }
+
+// deferStdout is used to make sure that nothing get emitted to stdout and stderr until intentionally rescued.
+// This is required to make sure that the plugin or its dependency doesn't interfere with notation <-> plugin communication
+func deferStdout() func() {
+	// Ignoring error because we don't want plugin to fail if `os.DevNull` is misconfigured.
+	null, _ := os.Open(os.DevNull)
+	sout := os.Stdout
+	serr := os.Stderr
+	os.Stdout = null
+	os.Stderr = null
+
+	return func() {
+		err := null.Close()
+		if err != nil {
+			return
+		}
+		os.Stdout = sout
+		os.Stderr = serr
+	}
+}
+
+// discardLogger implements Logger but logs nothing. It is used when user
+// disenabled logging option in notation, i.e. loggerKey is not in the context.
+type discardLogger struct{}
+
+func (dl *discardLogger) Debug(_ ...interface{}) {
+}
+
+func (dl *discardLogger) Debugf(_ string, _ ...interface{}) {
+}
+
+func (dl *discardLogger) Debugln(_ ...interface{}) {
+}
+
+func (dl *discardLogger) Info(_ ...interface{}) {
+}
+
+func (dl *discardLogger) Infof(_ string, _ ...interface{}) {
+}
+
+func (dl *discardLogger) Infoln(_ ...interface{}) {
+}
+
+func (dl *discardLogger) Warn(_ ...interface{}) {
+}
+
+func (dl *discardLogger) Warnf(_ string, _ ...interface{}) {
+}
+
+func (dl *discardLogger) Warnln(_ ...interface{}) {
+}
+
+func (dl *discardLogger) Error(_ ...interface{}) {
+}
+
+func (dl *discardLogger) Errorf(_ string, _ ...interface{}) {
+}
+
+func (dl *discardLogger) Errorln(_ ...interface{}) {
+}

example/main.go

@@ -18,7 +18,7 @@ func main() {
 	}
 
 	// Create executable
-	pluginCli, err := cli.New("notation-example", plugin)
+	pluginCli, err := cli.New(plugin)
 	if err != nil {
 		_, _ = fmt.Fprintf(os.Stderr, "failed to create executable: %v\n", err)
 		os.Exit(3)