Merge pull request #10 from vshn/flags

Improve handling around required flags

example_command.go

@@ -23,16 +23,18 @@ func newExampleCommand() *cli.Command {
 		Before: command.validate,
 		Action: command.execute,
 		Flags: []cli.Flag{
-			&cli.StringFlag{Destination: &command.ExampleFlag, Name: "flag", EnvVars: envVars("EXAMPLE_FLAG"), Value: "foo", Usage: "a demonstration how to configure the command"},
+			&cli.StringFlag{Destination: &command.ExampleFlag, Name: "flag", EnvVars: envVars("EXAMPLE_FLAG"), Value: "foo", Usage: "a demonstration how to configure the command", Required: true},
 		},
 	}
 }
 
 func (c *exampleCommand) validate(context *cli.Context) error {
+	_ = LogMetadata(context)
 	log := AppLogger(context).WithName(exampleCommandName)
 	log.V(1).Info("validating config")
-	if c.ExampleFlag == "" {
-		return fmt.Errorf("option cannot be empty: %s", "flag")
+	// The `Required` property in the StringFlag above already checks if it's non-empty.
+	if len(c.ExampleFlag) <= 2 {
+		return fmt.Errorf("option needs at least 3 characters: %s", "flag")
 	}
 	return nil
 }

example_command_test.go

@@ -14,7 +14,7 @@ func TestExampleCommand_Validate(t *testing.T) {
 	}{
 		// TODO: test cases
 		"GivenEmptyFlag_ThenExpectError": {
-			expectedError: "option cannot be empty: flag",
+			expectedError: "option needs at least 3 characters: flag",
 		},
 		"GivenValidConfig_ThenExpectNoError": {
 			givenExampleFlag: "test",

logger.go

@@ -2,6 +2,8 @@ package main
 
 import (
 	"log"
+	"os"
+	"runtime"
 	"strings"
 	"sync/atomic"
 
@@ -19,9 +21,29 @@ func AppLogger(c *cli.Context) logr.Logger {
 	return c.Context.Value(loggerContextKey{}).(*atomic.Value).Load().(logr.Logger)
 }
 
-func setupLogging(c *cli.Context) {
+// LogMetadata prints various metadata to the root logger.
+// It prints version, architecture and current user ID and returns nil.
+func LogMetadata(c *cli.Context) error {
+	logger := AppLogger(c)
+	if !usesProductionLoggingConfig(c) {
+		logger = logger.WithValues("version", version)
+	}
+	logger.WithValues(
+		"date", date,
+		"commit", commit,
+		"go_os", runtime.GOOS,
+		"go_arch", runtime.GOARCH,
+		"go_version", runtime.Version(),
+		"uid", os.Getuid(),
+		"gid", os.Getgid(),
+	).Info("Starting up " + appName)
+	return nil
+}
+
+func setupLogging(c *cli.Context) error {
 	logger := newZapLogger(appName, c.Bool("debug"), usesProductionLoggingConfig(c))
 	c.Context.Value(loggerContextKey{}).(*atomic.Value).Store(logger)
+	return nil
 }
 
 func usesProductionLoggingConfig(c *cli.Context) bool {

main.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"os"
 	"os/signal"
-	"runtime"
 	"sync/atomic"
 	"syscall"
 	"time"
@@ -32,7 +31,12 @@ var (
 func main() {
 	ctx, stop, app := newApp()
 	defer stop()
-	_ = app.RunContext(ctx, os.Args)
+	err := app.RunContext(ctx, os.Args)
+	// If required flags aren't set, it will return with error before we could set up logging
+	if err != nil {
+		_, _ = fmt.Fprintf(os.Stderr, "%v\n", err)
+		os.Exit(1)
+	}
 }
 
 func newApp() (context.Context, context.CancelFunc, *cli.App) {
@@ -46,7 +50,7 @@ func newApp() (context.Context, context.CancelFunc, *cli.App) {
 
 		EnableBashCompletion: true,
 
-		Before: beforeAction,
+		Before: setupLogging,
 		Flags: []cli.Flag{
 			&cli.BoolFlag{
 				Name:    "debug",
@@ -86,34 +90,8 @@ func rootAction(hasSubcommands bool) func(context *cli.Context) error {
 		if hasSubcommands {
 			return cli.ShowAppHelp(context)
 		}
-		logMetadata(context)
-		return nil
-	}
-}
-
-func beforeAction(c *cli.Context) error {
-	setupLogging(c)
-	if c.Args().Present() {
-		// only print metadata if not displaying usage
-		logMetadata(c)
-	}
-	return nil
-}
-
-func logMetadata(c *cli.Context) {
-	log := AppLogger(c)
-	if !usesProductionLoggingConfig(c) {
-		log = log.WithValues("version", version)
+		return LogMetadata(context)
 	}
-	log.WithValues(
-		"date", date,
-		"commit", commit,
-		"go_os", runtime.GOOS,
-		"go_arch", runtime.GOARCH,
-		"go_version", runtime.Version(),
-		"uid", os.Getuid(),
-		"gid", os.Getgid(),
-	).Info("Starting up " + appName)
 }
 
 // env combines envPrefix with given suffix delimited by underscore.

main_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"context"
+	"flag"
 	"sync/atomic"
 	"testing"
 
@@ -14,7 +15,7 @@ func newAppContext(t *testing.T) *cli.Context {
 	logger := zapr.NewLogger(zaptest.NewLogger(t))
 	instance := &atomic.Value{}
 	instance.Store(logger)
-	return cli.NewContext(&cli.App{}, nil, &cli.Context{
+	return cli.NewContext(&cli.App{}, flag.NewFlagSet("", flag.ContinueOnError), &cli.Context{
 		Context: context.WithValue(context.Background(), loggerContextKey{}, instance),
 	})
 }