From 06c35159b9511cfa3bffe3cba6ca3d18e799c83b Mon Sep 17 00:00:00 2001
From: Andrew Mason <andrew@planetscale.com>
Date: Mon, 8 Aug 2022 21:35:36 -0400
Subject: [PATCH] [cli] Migrate `vterrors` to `pflag` (#10957)

* Tidy imports, make logErrStacks private

Signed-off-by: Andrew Mason <andrew@planetscale.com>

* 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 https://github.com/vitessio/vitess/issues/10697#user-content-movers-guide)

Signed-off-by: Andrew Mason <andrew@planetscale.com>
---
 go/vt/servenv/servenv.go      |  7 +++++--
 go/vt/vterrors/errors_test.go | 11 +++++------
 go/vt/vterrors/vterrors.go    | 18 ++++++++++--------
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/go/vt/servenv/servenv.go b/go/vt/servenv/servenv.go
index 4c3519c503f..5c5414230a4 100644
--- 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
index 3020ace6942..96c034c45ee 100644
--- a/go/vt/vterrors/errors_test.go
+++ b/go/vt/vterrors/errors_test.go
@@ -17,6 +17,7 @@ limitations under the License.
 package vterrors
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"io"
@@ -24,8 +25,6 @@ import (
 	"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
index 4a2bdf39004..cc66b62e8d3 100644
--- 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