[cli] Migrate vterrors to pflag (#10957)

* Tidy imports, make logErrStacks private Signed-off-by: Andrew Mason <[email protected]> * Migrate vterrors to `pflag` Since every binary will involve errors in some way, shape, or form, I decided to just register the vterrors flags to all binaries, rather than naming each binary explicitly. This notably does the _inverse_ of the code samples in the [Mover's Guide][1], because `servenv` has `vterrors` in its dependency chain, so trying to import `servenv` from `vterrors` would cause an import cycle. This is the approach we should continue to use for any vitess package imported by `servenv`. [1]: see #10697 (comment)) Signed-off-by: Andrew Mason <[email protected]>
vitessio · Aug 9, 2022 · 06c3515 · 06c3515
1 parent 7f25195
commit 06c3515
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 16 deletions.
diff --git a/go/vt/servenv/servenv.go b/go/vt/servenv/servenv.go
@@ -47,6 +47,7 @@ import (
 	"vitess.io/vitess/go/netutil"
 	"vitess.io/vitess/go/stats"
 	"vitess.io/vitess/go/vt/log"
+	"vitess.io/vitess/go/vt/vterrors"
 
 	// register the proper init and shutdown hooks for logging
 	_ "vitess.io/vitess/go/vt/logutil"
@@ -232,8 +233,10 @@ func RunDefault() {
 }
 
 var (
-	flagHooksM       sync.Mutex
-	globalFlagHooks  []func(*pflag.FlagSet)
+	flagHooksM      sync.Mutex
+	globalFlagHooks = []func(*pflag.FlagSet){
+		vterrors.RegisterFlags,
+	}
 	commandFlagHooks = map[string][]func(*pflag.FlagSet){}
 )
 

diff --git a/go/vt/vterrors/errors_test.go b/go/vt/vterrors/errors_test.go
@@ -17,15 +17,14 @@ limitations under the License.
 package vterrors
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"io"
 	"reflect"
 	"strings"
 	"testing"
 
-	"context"
-
 	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
 )
 
@@ -193,8 +192,8 @@ func TestStackFormat(t *testing.T) {
 	assertContains(t, got, "middle", false)
 	assertContains(t, got, "outer", false)
 
-	LogErrStacks = true
-	defer func() { LogErrStacks = false }()
+	logErrStacks = true
+	defer func() { logErrStacks = false }()
 	got = fmt.Sprintf("%v", err)
 	assertContains(t, got, "innerMost", true)
 	assertContains(t, got, "middle", true)
@@ -276,9 +275,9 @@ func TestWrapping(t *testing.T) {
 	err3 := Wrapf(err2, "baz")
 	errorWithoutStack := fmt.Sprintf("%v", err3)
 
-	LogErrStacks = true
+	logErrStacks = true
 	errorWithStack := fmt.Sprintf("%v", err3)
-	LogErrStacks = false
+	logErrStacks = false
 
 	assertEquals(t, err3.Error(), "baz: bar: foo")
 	assertContains(t, errorWithoutStack, "foo", true)

diff --git a/go/vt/vterrors/vterrors.go b/go/vt/vterrors/vterrors.go
@@ -86,21 +86,23 @@ limitations under the License.
 package vterrors
 
 import (
-	"flag"
+	"context"
 	"fmt"
 	"io"
 
-	"context"
+	"github.com/spf13/pflag"
 
 	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
 )
 
-// LogErrStacks controls whether or not printing errors includes the
+// logErrStacks controls whether or not printing errors includes the
 // embedded stack trace in the output.
-var LogErrStacks bool
+var logErrStacks bool
 
-func init() {
-	flag.BoolVar(&LogErrStacks, "log_err_stacks", false, "log stack traces for errors")
+// RegisterFlags registers the command-line options that control vterror
+// behavior on the provided FlagSet.
+func RegisterFlags(fs *pflag.FlagSet) {
+	fs.BoolVar(&logErrStacks, "log_err_stacks", false, "log stack traces for errors")
 }
 
 // New returns an error with the supplied message.
@@ -153,7 +155,7 @@ func (f *fundamental) Format(s fmt.State, verb rune) {
 	case 'v':
 		panicIfError(io.WriteString(s, "Code: "+f.code.String()+"\n"))
 		panicIfError(io.WriteString(s, f.msg+"\n"))
-		if LogErrStacks {
+		if logErrStacks {
 			f.stack.Format(s, verb)
 		}
 		return
@@ -249,7 +251,7 @@ func (w *wrapping) Format(s fmt.State, verb rune) {
 	if rune('v') == verb {
 		panicIfError(fmt.Fprintf(s, "%v\n", w.Cause()))
 		panicIfError(io.WriteString(s, w.msg))
-		if LogErrStacks {
+		if logErrStacks {
 			w.stack.Format(s, verb)
 		}
 		return