From d1cd456a1be0e0da71df2cfea78a96c2d98ebed9 Mon Sep 17 00:00:00 2001
From: Raphael 'kena' Poss <knz@thaumogen.net>
Date: Sun, 28 Feb 2021 14:26:54 +0100
Subject: [PATCH] cli: ensure that `--log` is recognized for non-server
 commands

Release justification: bug fixes and low-risk updates to new functionality

Release note (bug fix): The non-server `cockroach` commands now
recognize the new `--log` flag properly. This had been broken
unadvertently in one of the earlier 21.1 alpha releases.
---
 pkg/acceptance/cluster/dockercluster.go      |  6 ++++--
 pkg/cli/cli.go                               | 21 +++++++++++++++++---
 pkg/cli/interactive_tests/test_log_flags.tcl | 18 +++++++++++++++++
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/pkg/acceptance/cluster/dockercluster.go b/pkg/acceptance/cluster/dockercluster.go
index b18ebcc54b80..682ebd71b88c 100644
--- a/pkg/acceptance/cluster/dockercluster.go
+++ b/pkg/acceptance/cluster/dockercluster.go
@@ -523,7 +523,8 @@ func (l *DockerCluster) RunInitCommand(ctx context.Context, nodeIdx int) {
 			"init",
 			"--certs-dir=/certs/",
 			"--host=" + l.Nodes[nodeIdx].nodeStr,
-			"--logtostderr",
+			"--log-dir=/logs/init-command",
+			"--logtostderr=NONE",
 		},
 	}
 
@@ -639,14 +640,15 @@ func (l *DockerCluster) Start(ctx context.Context) {
 	log.Infof(ctx, "creating node certs (%dbit) in: %s", keyLen, certsDir)
 	l.createNodeCerts()
 
+	log.Infof(ctx, "starting %d nodes", len(l.Nodes))
 	l.monitorCtx, l.monitorCtxCancelFunc = context.WithCancel(context.Background())
 	go l.monitor(ctx)
 	var wg sync.WaitGroup
 	wg.Add(len(l.Nodes))
 	for _, node := range l.Nodes {
 		go func(node *testNode) {
+			defer wg.Done()
 			l.startNode(ctx, node)
-			wg.Done()
 		}(node)
 	}
 	wg.Wait()
diff --git a/pkg/cli/cli.go b/pkg/cli/cli.go
index 8318147493aa..bec94116adc9 100644
--- a/pkg/cli/cli.go
+++ b/pkg/cli/cli.go
@@ -81,9 +81,24 @@ func Main() {
 func doMain(cmd *cobra.Command, cmdName string) error {
 	if cmd != nil && !cmdHasCustomLoggingSetup(cmd) {
 		// the customLoggingSetupCmds do their own calls to setupLogging().
-		if err := setupLogging(context.Background(), cmd,
-			false /* isServerCmd */, true /* applyConfig */); err != nil {
-			return err
+		//
+		// We use a PreRun function, to ensure setupLogging() is only
+		// called after the command line flags have been parsed.
+		//
+		// NB: we cannot use PersistentPreRunE,like in flags.go, because
+		// overriding that here will prevent the persistent pre-run from
+		// running on parent commands. (See the difference between PreRun
+		// and PersistentPreRun in `(*cobra.Command) execute()`.)
+		wrapped := cmd.PreRunE
+		cmd.PreRunE = func(cmd *cobra.Command, args []string) error {
+			if wrapped != nil {
+				if err := wrapped(cmd, args); err != nil {
+					return err
+				}
+			}
+
+			return setupLogging(context.Background(), cmd,
+				false /* isServerCmd */, true /* applyConfig */)
 		}
 	}
 
diff --git a/pkg/cli/interactive_tests/test_log_flags.tcl b/pkg/cli/interactive_tests/test_log_flags.tcl
index e0b2301ff751..d55fc3934233 100644
--- a/pkg/cli/interactive_tests/test_log_flags.tcl
+++ b/pkg/cli/interactive_tests/test_log_flags.tcl
@@ -63,5 +63,23 @@ eexpect "parsing \"cantparse\": invalid syntax"
 eexpect ":/# "
 end_test
 
+start_test "Check that conflicting legacy and new flags are properly rejected for server commands"
+send "$argv start-single-node --insecure --logtostderr=true --log=abc\r"
+eexpect "log is incompatible with legacy discrete logging flag"
+eexpect ":/# "
+end_test
+
+start_test "Check that conflicting legacy and new flags are properly rejected for client commands"
+send "$argv sql --insecure --logtostderr=true --log=abc\r"
+eexpect "log is incompatible with legacy discrete logging flag"
+eexpect ":/# "
+end_test
+
+start_test "Check that the log flag is properly recognized for non-server commands"
+send "$argv debug reset-quorum 123 --log='sinks: {stderr: {format: json }}'\r"
+eexpect "\"severity\":\"ERROR\""
+eexpect "connection to server failed"
+eexpect ":/# "
+
 send "exit 0\r"
 eexpect eof