Repository: GoogleCloudPlatform/terraform-validator
Pull request #321 (2021-09-28 22:26:23)

Switched to verbosity flag
This replaces the boolean --verbose flag. Also, it turns out that the flag values actually get extracted from the command line during the Execute step, so the value of `--verbose` wasn't actually being used to initialize the loggers. I moved the logger initialization into PersistentPreRun to resolve this issue, since that [runs first](https://github.com/spf13/cobra/blob/4fd30b69ee2b62cf3bbecf0a423f8a1ee47f5f24/command.go#L98).

Note: About 1/3 of this PR is whitespace changes - reviewing with whitespace changes hidden might be easier.


Patch:
diff --git a/cmd/convert.go b/cmd/convert.go
index 557fe3f2..d587a9ae 100644
--- a/cmd/convert.go
+++ b/cmd/convert.go
@@ -41,21 +41,17 @@ Example:
 `
 
 type convertOptions struct {
-	project              string
-	ancestry             string
-	offline              bool
-	errorLogger          *zap.Logger
-	outputLogger         *zap.Logger
-	useStructuredLogging bool
-	readPlannedAssets    tfgcv.ReadPlannedAssetsFunc
+	project           string
+	ancestry          string
+	offline           bool
+	rootOptions       *rootOptions
+	readPlannedAssets tfgcv.ReadPlannedAssetsFunc
 }
 
-func newConvertCmd(errorLogger, outputLogger *zap.Logger, useStructuredLogging bool) *cobra.Command {
+func newConvertCmd(rootOptions *rootOptions) *cobra.Command {
 	o := &convertOptions{
-		errorLogger:          errorLogger,
-		outputLogger:         outputLogger,
-		useStructuredLogging: useStructuredLogging,
-		readPlannedAssets:    tfgcv.ReadPlannedAssets,
+		rootOptions:       rootOptions,
+		readPlannedAssets: tfgcv.ReadPlannedAssets,
 	}
 
 	cmd := &cobra.Command{
@@ -89,7 +85,7 @@ func (o *convertOptions) validateArgs(args []string) error {
 
 func (o *convertOptions) run(plan string) error {
 	ctx := context.Background()
-	assets, err := o.readPlannedAssets(ctx, plan, o.project, o.ancestry, o.offline, false, o.errorLogger)
+	assets, err := o.readPlannedAssets(ctx, plan, o.project, o.ancestry, o.offline, false, o.rootOptions.errorLogger)
 	if err != nil {
 		if errors.Cause(err) == tfgcv.ErrParsingProviderProject {
 			return errors.New("unable to parse provider project, please use --project flag")
@@ -97,8 +93,8 @@ func (o *convertOptions) run(plan string) error {
 		return errors.Wrap(err, "converting tfplan to CAI assets")
 	}
 
-	if o.useStructuredLogging {
-		o.outputLogger.Info(
+	if o.rootOptions.useStructuredLogging {
+		o.rootOptions.outputLogger.Info(
 			"converted resources",
 			zap.Any("resource_body", assets),
 		)
diff --git a/cmd/logger.go b/cmd/logger.go
index 704430e5..998b676f 100644
--- a/cmd/logger.go
+++ b/cmd/logger.go
@@ -66,9 +66,9 @@ func newConsoleEncoder(cfg zapcore.EncoderConfig) errorEncoder {
 	}
 }
 
-func newErrorLogger(verbose, useStructuredLogging bool, writeSyncer zapcore.WriteSyncer) *zap.Logger {
+func newErrorLogger(verbosity string, useStructuredLogging bool, writeSyncer zapcore.WriteSyncer) *zap.Logger {
 	// Return a logger that produces expected structured output format for errors
-	var level zap.AtomicLevel
+	var level zapcore.LevelEnabler
 	options := []zap.Option{
 		zap.Fields(
 			// Message format version
@@ -76,12 +76,25 @@ func newErrorLogger(verbose, useStructuredLogging bool, writeSyncer zapcore.Writ
 		),
 	}
 
-	if verbose {
-		level = zap.NewAtomicLevelAt(zap.DebugLevel)
+	switch verbosity {
+	case "debug":
+		level = zap.DebugLevel
 		options = append(options, zap.AddStacktrace(zap.WarnLevel))
-	} else {
-		level = zap.NewAtomicLevelAt(zap.InfoLevel)
+	// case "info" is handled by default
+	case "warning":
+		level = zap.WarnLevel
+		options = append(options, zap.AddStacktrace(zap.ErrorLevel))
+	case "error":
+		level = zap.ErrorLevel
 		options = append(options, zap.AddStacktrace(zap.ErrorLevel))
+	case "critical":
+		level = zap.PanicLevel
+		options = append(options, zap.AddStacktrace(zap.PanicLevel))
+	case "none":
+		return zap.NewNop()
+	default:
+		level = zap.InfoLevel
+		options = append(options, zap.AddStacktrace(zap.WarnLevel))
 	}
 
 	encoderConfig := zapcore.EncoderConfig{
diff --git a/cmd/root.go b/cmd/root.go
index 56051d49..f375639b 100644
--- a/cmd/root.go
+++ b/cmd/root.go
@@ -30,13 +30,26 @@ policy library written to expect Google CAI (Cloud Asset Inventory) data.
 
 Supported Terraform versions = 0.12+`
 
+var allowedVerbosity = map[string]struct{}{
+	"debug":    struct{}{},
+	"info":     struct{}{},
+	"warning":  struct{}{},
+	"error":    struct{}{},
+	"critical": struct{}{},
+	"none":     struct{}{},
+}
+
 type rootOptions struct {
-	verbose     bool
-	errorLogger *zap.Logger
+	verbosity            string
+	errorLogger          *zap.Logger
+	outputLogger         *zap.Logger
+	useStructuredLogging bool
 }
 
-func newRootCmd() (*cobra.Command, *zap.Logger, error) {
-	o := &rootOptions{}
+func newRootCmd() (*cobra.Command, *rootOptions, error) {
+	o := &rootOptions{
+		useStructuredLogging: os.Getenv("USE_STRUCTURED_LOGGING") == "true",
+	}
 
 	cmd := &cobra.Command{
 		Use:           "terraform-validator",
@@ -44,31 +57,40 @@ func newRootCmd() (*cobra.Command, *zap.Logger, error) {
 		Long:          rootCmdDesc,
 		SilenceUsage:  true,
 		SilenceErrors: true,
+		PersistentPreRunE: func(c *cobra.Command, args []string) error {
+			if _, ok := allowedVerbosity[o.verbosity]; !ok {
+				return errors.New("verbosity must be one of: debug, info, warning, error, critical, none.")
+			}
+
+			// set this up in PersistentPreRun because we need to wait for flags to be parsed
+			// to have accurate verbosity.
+			errorLogger := newErrorLogger(o.verbosity, o.useStructuredLogging, zapcore.Lock(os.Stderr))
+			defer errorLogger.Sync()
+			zap.RedirectStdLog(errorLogger)
+			o.errorLogger = errorLogger
+
+			outputLogger := newOutputLogger(zapcore.Lock(os.Stdout))
+			defer outputLogger.Sync()
+			o.outputLogger = outputLogger
+
+			return nil
+		},
 	}
 
-	cmd.PersistentFlags().BoolVar(&o.verbose, "verbose", false, "Log additional output")
-
-	useStructuredLogging := os.Getenv("USE_STRUCTURED_LOGGING") == "true"
-	errorLogger := newErrorLogger(o.verbose, useStructuredLogging, zapcore.Lock(os.Stderr))
-	defer errorLogger.Sync()
-	zap.RedirectStdLog(errorLogger)
-	o.errorLogger = errorLogger
-
-	outputLogger := newOutputLogger(zapcore.Lock(os.Stdout))
-	defer outputLogger.Sync()
+	cmd.PersistentFlags().StringVar(&o.verbosity, "verbosity", "info", "Set verbosity level. One of: debug, info, warning, error, critical, none.")
 
-	cmd.AddCommand(newConvertCmd(errorLogger, outputLogger, useStructuredLogging))
+	cmd.AddCommand(newConvertCmd(o))
 	cmd.AddCommand(newListSupportedResourcesCmd())
-	cmd.AddCommand(newValidateCmd(errorLogger, outputLogger, useStructuredLogging))
+	cmd.AddCommand(newValidateCmd(o))
 	cmd.AddCommand(newVersionCmd())
 
-	return cmd, errorLogger, nil
+	return cmd, o, nil
 }
 
 // Execute is the entry-point for all commands.
 // This lets us keep all new command functions private.
 func Execute() {
-	rootCmd, logger, err := newRootCmd()
+	rootCmd, rootOptions, err := newRootCmd()
 
 	if err != nil {
 		fmt.Printf("Error creating root logger: %s", err)
@@ -82,7 +104,11 @@ func Execute() {
 	} else if errors.Is(err, errViolations) {
 		os.Exit(2)
 	} else {
-		logger.Error(err.Error())
+		if rootOptions.errorLogger == nil {
+			fmt.Println(err.Error())
+		} else {
+			rootOptions.errorLogger.Error(err.Error())
+		}
 		os.Exit(1)
 	}
 }
diff --git a/cmd/validate.go b/cmd/validate.go
index 2fa3b6e5..405b16da 100644
--- a/cmd/validate.go
+++ b/cmd/validate.go
@@ -42,26 +42,22 @@ Example:
 `
 
 type validateOptions struct {
-	project              string
-	ancestry             string
-	offline              bool
-	policyPath           string
-	outputJSON           bool
-	dryRun               bool
-	errorLogger          *zap.Logger
-	outputLogger         *zap.Logger
-	useStructuredLogging bool
-	readPlannedAssets    tfgcv.ReadPlannedAssetsFunc
-	validateAssets       tfgcv.ValidateAssetsFunc
+	project           string
+	ancestry          string
+	offline           bool
+	policyPath        string
+	outputJSON        bool
+	dryRun            bool
+	rootOptions       *rootOptions
+	readPlannedAssets tfgcv.ReadPlannedAssetsFunc
+	validateAssets    tfgcv.ValidateAssetsFunc
 }
 
-func newValidateCmd(errorLogger, outputLogger *zap.Logger, useStructuredLogging bool) *cobra.Command {
+func newValidateCmd(rootOptions *rootOptions) *cobra.Command {
 	o := &validateOptions{
-		errorLogger:          errorLogger,
-		outputLogger:         outputLogger,
-		useStructuredLogging: useStructuredLogging,
-		readPlannedAssets:    tfgcv.ReadPlannedAssets,
-		validateAssets:       tfgcv.ValidateAssets,
+		rootOptions:       rootOptions,
+		readPlannedAssets: tfgcv.ReadPlannedAssets,
+		validateAssets:    tfgcv.ValidateAssets,
 	}
 
 	cmd := &cobra.Command{
@@ -103,7 +99,7 @@ func (o *validateOptions) validateArgs(args []string) error {
 
 func (o *validateOptions) run(plan string) error {
 	ctx := context.Background()
-	assets, err := o.readPlannedAssets(ctx, plan, o.project, o.ancestry, o.offline, false, o.errorLogger)
+	assets, err := o.readPlannedAssets(ctx, plan, o.project, o.ancestry, o.offline, false, o.rootOptions.errorLogger)
 	if err != nil {
 		if errors.Cause(err) == tfgcv.ErrParsingProviderProject {
 			return errors.New("unable to parse provider project, please use --project flag")
@@ -116,12 +112,12 @@ func (o *validateOptions) run(plan string) error {
 		return errors.Wrap(err, "validating: FCV")
 	}
 
-	if o.useStructuredLogging {
+	if o.rootOptions.useStructuredLogging {
 		msg := "No violations found"
 		if len(auditResult.Violations) > 0 {
 			msg = "Violations found"
 		}
-		o.outputLogger.Info(
+		o.rootOptions.outputLogger.Info(
 			msg,
 			zap.Any("resource_body", auditResult.Violations),
 		)


Test patch:
diff --git a/cmd/convert_test.go b/cmd/convert_test.go
index 8316101e..2fa948a0 100644
--- a/cmd/convert_test.go
+++ b/cmd/convert_test.go
@@ -40,18 +40,22 @@ func MockReadPlannedAssets(ctx context.Context, path, project, ancestry string,
 
 func TestConvertRun(t *testing.T) {
 	a := assert.New(t)
-	verbose := true
+	verbosity := "debug"
 	useStructuredLogging := true
-	errorLogger, errorBuf := newTestErrorLogger(verbose, useStructuredLogging)
+	errorLogger, errorBuf := newTestErrorLogger(verbosity, useStructuredLogging)
 	outputLogger, outputBuf := newTestOutputLogger()
-	o := convertOptions{
-		project:              "",
-		ancestry:             "",
-		offline:              false,
+	ro := &rootOptions{
+		verbosity:            verbosity,
+		useStructuredLogging: useStructuredLogging,
 		errorLogger:          errorLogger,
 		outputLogger:         outputLogger,
-		useStructuredLogging: useStructuredLogging,
-		readPlannedAssets:    MockReadPlannedAssets,
+	}
+	o := convertOptions{
+		project:           "",
+		ancestry:          "",
+		offline:           false,
+		rootOptions:       ro,
+		readPlannedAssets: MockReadPlannedAssets,
 	}
 
 	err := o.run("/path/to/plan")
@@ -77,18 +81,22 @@ func TestConvertRun(t *testing.T) {
 
 func TestConvertRunLegacy(t *testing.T) {
 	a := assert.New(t)
-	verbose := true
+	verbosity := "debug"
 	useStructuredLogging := false
-	errorLogger, errorBuf := newTestErrorLogger(verbose, useStructuredLogging)
+	errorLogger, errorBuf := newTestErrorLogger(verbosity, useStructuredLogging)
 	outputLogger, outputBuf := newTestOutputLogger()
-	o := convertOptions{
-		project:              "",
-		ancestry:             "",
-		offline:              false,
+	ro := &rootOptions{
+		verbosity:            verbosity,
+		useStructuredLogging: useStructuredLogging,
 		errorLogger:          errorLogger,
 		outputLogger:         outputLogger,
-		useStructuredLogging: useStructuredLogging,
-		readPlannedAssets:    MockReadPlannedAssets,
+	}
+	o := convertOptions{
+		project:           "",
+		ancestry:          "",
+		offline:           false,
+		rootOptions:       ro,
+		readPlannedAssets: MockReadPlannedAssets,
 	}
 
 	err := o.run("/path/to/plan")
diff --git a/cmd/logger_test.go b/cmd/logger_test.go
index 7c15f1ee..bfb23003 100644
--- a/cmd/logger_test.go
+++ b/cmd/logger_test.go
@@ -18,10 +18,10 @@ func (bws bufferWriteSyncer) Sync() error {
 	return nil
 }
 
-func newTestErrorLogger(verbose, useStructuredLogging bool) (*zap.Logger, *bytes.Buffer) {
+func newTestErrorLogger(verbosity string, useStructuredLogging bool) (*zap.Logger, *bytes.Buffer) {
 	buf := new(bytes.Buffer)
 	syncer := bufferWriteSyncer{Buffer: buf}
-	logger := newErrorLogger(verbose, useStructuredLogging, syncer)
+	logger := newErrorLogger(verbosity, useStructuredLogging, syncer)
 	return logger, syncer.Buffer
 }
 
@@ -42,10 +42,10 @@ func TestErrorLoggerSchema(t *testing.T) {
 	//         "context": "additional error context (optional)"
 	//     }
 	// }
-	verbose := true
+	verbosity := "debug"
 	useStructuredLogging := true
 
-	errorLogger, buf := newTestErrorLogger(verbose, useStructuredLogging)
+	errorLogger, buf := newTestErrorLogger(verbosity, useStructuredLogging)
 	errorLogger.Info("This is a message")
 
 	outputJSON := buf.Bytes()
diff --git a/cmd/validate_test.go b/cmd/validate_test.go
index d2f7cc05..bce72700 100644
--- a/cmd/validate_test.go
+++ b/cmd/validate_test.go
@@ -39,22 +39,26 @@ func MockValidateAssetsWithViolations(ctx context.Context, assets []google.Asset
 
 func TestValidateRun(t *testing.T) {
 	a := assert.New(t)
-	verbose := true
+	verbosity := "debug"
 	useStructuredLogging := true
-	errorLogger, errorBuf := newTestErrorLogger(verbose, useStructuredLogging)
+	errorLogger, errorBuf := newTestErrorLogger(verbosity, useStructuredLogging)
 	outputLogger, outputBuf := newTestOutputLogger()
-	o := validateOptions{
-		project:              "",
-		ancestry:             "",
-		offline:              false,
-		policyPath:           "",
-		outputJSON:           false,
-		dryRun:               false,
+	ro := &rootOptions{
+		verbosity:            verbosity,
+		useStructuredLogging: useStructuredLogging,
 		errorLogger:          errorLogger,
 		outputLogger:         outputLogger,
-		useStructuredLogging: useStructuredLogging,
-		readPlannedAssets:    MockReadPlannedAssets,
-		validateAssets:       MockValidateAssetsWithViolations,
+	}
+	o := validateOptions{
+		project:           "",
+		ancestry:          "",
+		offline:           false,
+		policyPath:        "",
+		outputJSON:        false,
+		dryRun:            false,
+		rootOptions:       ro,
+		readPlannedAssets: MockReadPlannedAssets,
+		validateAssets:    MockValidateAssetsWithViolations,
 	}
 
 	err := o.run("/path/to/plan")
@@ -80,22 +84,26 @@ func TestValidateRun(t *testing.T) {
 
 func TestValidateRunNoViolations(t *testing.T) {
 	a := assert.New(t)
-	verbose := true
+	verbosity := "debug"
 	useStructuredLogging := true
-	errorLogger, errorBuf := newTestErrorLogger(verbose, useStructuredLogging)
+	errorLogger, errorBuf := newTestErrorLogger(verbosity, useStructuredLogging)
 	outputLogger, outputBuf := newTestOutputLogger()
-	o := validateOptions{
-		project:              "",
-		ancestry:             "",
-		offline:              false,
-		policyPath:           "",
-		outputJSON:           false,
-		dryRun:               false,
+	ro := &rootOptions{
+		verbosity:            verbosity,
+		useStructuredLogging: useStructuredLogging,
 		errorLogger:          errorLogger,
 		outputLogger:         outputLogger,
-		useStructuredLogging: useStructuredLogging,
-		readPlannedAssets:    MockReadPlannedAssets,
-		validateAssets:       MockValidateAssetsNoViolations,
+	}
+	o := validateOptions{
+		project:           "",
+		ancestry:          "",
+		offline:           false,
+		policyPath:        "",
+		outputJSON:        false,
+		dryRun:            false,
+		rootOptions:       ro,
+		readPlannedAssets: MockReadPlannedAssets,
+		validateAssets:    MockValidateAssetsNoViolations,
 	}
 
 	err := o.run("/path/to/plan")
@@ -116,22 +124,26 @@ func TestValidateRunNoViolations(t *testing.T) {
 
 func TestValidateRunLegacy(t *testing.T) {
 	a := assert.New(t)
-	verbose := true
+	verbosity := "debug"
 	useStructuredLogging := false
-	errorLogger, errorBuf := newTestErrorLogger(verbose, useStructuredLogging)
+	errorLogger, errorBuf := newTestErrorLogger(verbosity, useStructuredLogging)
 	outputLogger, outputBuf := newTestOutputLogger()
-	o := validateOptions{
-		project:              "",
-		ancestry:             "",
-		offline:              false,
-		policyPath:           "",
-		outputJSON:           false,
-		dryRun:               false,
+	ro := &rootOptions{
+		verbosity:            verbosity,
+		useStructuredLogging: useStructuredLogging,
 		errorLogger:          errorLogger,
 		outputLogger:         outputLogger,
-		useStructuredLogging: useStructuredLogging,
-		readPlannedAssets:    MockReadPlannedAssets,
-		validateAssets:       MockValidateAssetsWithViolations,
+	}
+	o := validateOptions{
+		project:           "",
+		ancestry:          "",
+		offline:           false,
+		policyPath:        "",
+		outputJSON:        false,
+		dryRun:            false,
+		rootOptions:       ro,
+		readPlannedAssets: MockReadPlannedAssets,
+		validateAssets:    MockValidateAssetsWithViolations,
 	}
 
 	err := o.run("/path/to/plan")
@@ -147,22 +159,26 @@ func TestValidateRunLegacy(t *testing.T) {
 
 func TestValidateRunNoViolationsLegacy(t *testing.T) {
 	a := assert.New(t)
-	verbose := true
+	verbosity := "debug"
 	useStructuredLogging := false
-	errorLogger, errorBuf := newTestErrorLogger(verbose, useStructuredLogging)
+	errorLogger, errorBuf := newTestErrorLogger(verbosity, useStructuredLogging)
 	outputLogger, outputBuf := newTestOutputLogger()
-	o := validateOptions{
-		project:              "",
-		ancestry:             "",
-		offline:              false,
-		policyPath:           "",
-		outputJSON:           false,
-		dryRun:               false,
+	ro := &rootOptions{
+		verbosity:            verbosity,
+		useStructuredLogging: useStructuredLogging,
 		errorLogger:          errorLogger,
 		outputLogger:         outputLogger,
-		useStructuredLogging: useStructuredLogging,
-		readPlannedAssets:    MockReadPlannedAssets,
-		validateAssets:       MockValidateAssetsNoViolations,
+	}
+	o := validateOptions{
+		project:           "",
+		ancestry:          "",
+		offline:           false,
+		policyPath:        "",
+		outputJSON:        false,
+		dryRun:            false,
+		rootOptions:       ro,
+		readPlannedAssets: MockReadPlannedAssets,
+		validateAssets:    MockValidateAssetsNoViolations,
 	}
 
 	err := o.run("/path/to/plan")