Merge #67457

67457: cli: extract the SQL connection code to a new package `clisqlclient` (2/) r=otan a=knz This is part of a family of PRs that aim to encapsulate CLI functionality into finer grained packages. Co-authored-by: Raphael 'kena' Poss <[email protected]>
cockroachdb · Jul 14, 2021 · 162a1d4 · 162a1d4
2 parents 5a0b11e + ae9938c
commit 162a1d4
Show file tree

Hide file tree

Showing 51 changed files with 1,621 additions and 891 deletions.
diff --git a/Makefile b/Makefile
@@ -981,7 +981,7 @@ $(COCKROACH) $(COCKROACHOSS) go-install: override LINKFLAGS += \
 settings-doc-gen = $(if $(filter buildshort,$(MAKECMDGOALS)),$(COCKROACHSHORT),$(COCKROACH))
 
 docs/generated/settings/settings.html: $(settings-doc-gen)
-	@$(settings-doc-gen) gen settings-list --format=html > $@
+	@$(settings-doc-gen) gen settings-list --format=rawhtml > $@
 
 docs/generated/settings/settings-for-tenants.txt:  $(settings-doc-gen)
 	@$(settings-doc-gen) gen settings-list --without-system-only > $@

diff --git a/pkg/BUILD.bazel b/pkg/BUILD.bazel
@@ -45,6 +45,7 @@ ALL_TESTS = [
     "//pkg/ccl/utilccl:utilccl_test",
     "//pkg/ccl/workloadccl/allccl:allccl_test",
     "//pkg/ccl/workloadccl:workloadccl_test",
+    "//pkg/cli/clierror:clierror_test",
     "//pkg/cli/clisqlclient:clisqlclient_test",
     "//pkg/cli/exit:exit_test",
     "//pkg/cli:cli_test",

diff --git a/pkg/ccl/cliccl/BUILD.bazel b/pkg/ccl/cliccl/BUILD.bazel
@@ -23,6 +23,7 @@ go_library(
         "//pkg/ccl/workloadccl/cliccl",
         "//pkg/cli",
         "//pkg/cli/cliflags",
+        "//pkg/cli/clisqlclient",
         "//pkg/keys",
         "//pkg/roachpb",
         "//pkg/security",
@@ -71,6 +72,7 @@ go_test(
         "//pkg/ccl/backupccl",
         "//pkg/ccl/utilccl",
         "//pkg/cli",
+        "//pkg/cli/clisqlclient",
         "//pkg/server",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",

diff --git a/pkg/ccl/cliccl/debug_backup.go b/pkg/ccl/cliccl/debug_backup.go
@@ -22,13 +22,14 @@ import (
 	"strings"
 	"time"
 
-	"github.com/cockroachdb/apd/v2"
+	apd "github.com/cockroachdb/apd/v2"
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/blobs"
 	"github.com/cockroachdb/cockroach/pkg/ccl/backupccl"
 	"github.com/cockroachdb/cockroach/pkg/ccl/storageccl"
 	"github.com/cockroachdb/cockroach/pkg/cli"
 	"github.com/cockroachdb/cockroach/pkg/cli/cliflags"
+	"github.com/cockroachdb/cockroach/pkg/cli/clisqlclient"
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security"
@@ -342,7 +343,7 @@ func runListBackupsCmd(cmd *cobra.Command, args []string) error {
 	for _, backupPath := range backupPaths {
 		rows = append(rows, []string{"." + backupPath})
 	}
-	rowSliceIter := cli.NewRowSliceIter(rows, "l" /*align*/)
+	rowSliceIter := clisqlclient.NewRowSliceIter(rows, "l" /*align*/)
 	return cli.PrintQueryOutput(os.Stdout, cols, rowSliceIter)
 }
 
@@ -400,7 +401,7 @@ func runListIncrementalCmd(cmd *cobra.Command, args []string) error {
 		rows = append(rows, newRow)
 	}
 	cols := []string{"path", "start time", "end time"}
-	rowSliceIter := cli.NewRowSliceIter(rows, "lll" /*align*/)
+	rowSliceIter := clisqlclient.NewRowSliceIter(rows, "lll" /*align*/)
 	return cli.PrintQueryOutput(os.Stdout, cols, rowSliceIter)
 }
 

diff --git a/pkg/ccl/cliccl/debug_backup_test.go b/pkg/ccl/cliccl/debug_backup_test.go
@@ -20,6 +20,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/build"
 	"github.com/cockroachdb/cockroach/pkg/ccl/backupccl"
 	"github.com/cockroachdb/cockroach/pkg/cli"
+	"github.com/cockroachdb/cockroach/pkg/cli/clisqlclient"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
@@ -185,7 +186,7 @@ func TestListBackups(t *testing.T) {
 			{"." + ts[2].GoTime().Format(backupccl.DateBasedIntoFolderName)},
 		}
 		cols := []string{"path"}
-		rowSliceIter := cli.NewRowSliceIter(rows, "l" /*align*/)
+		rowSliceIter := clisqlclient.NewRowSliceIter(rows, "l" /*align*/)
 		if err := cli.PrintQueryOutput(&buf, cols, rowSliceIter); err != nil {
 			t.Fatalf("TestListBackups: PrintQueryOutput: %v", err)
 		}
@@ -227,7 +228,7 @@ func TestListIncremental(t *testing.T) {
 		{"/fooFolder" + expectedIncFolder2, ts[1].GoTime().Format(time.RFC3339), ts[2].GoTime().Format(time.RFC3339)},
 	}
 	cols := []string{"path", "start time", "end time"}
-	rowSliceIter := cli.NewRowSliceIter(rows, "lll" /*align*/)
+	rowSliceIter := clisqlclient.NewRowSliceIter(rows, "lll" /*align*/)
 	if err := cli.PrintQueryOutput(&buf, cols, rowSliceIter); err != nil {
 		t.Fatalf("TestListIncremental: PrintQueryOutput: %v", err)
 	}

diff --git a/pkg/cli/BUILD.bazel b/pkg/cli/BUILD.bazel
@@ -6,6 +6,7 @@ go_library(
     srcs = [
         "absolute_fs.go",
         "auth.go",
+        "binding.go",
         "cert.go",
         "cli.go",
         "client_url.go",
@@ -46,8 +47,7 @@ go_library(
         "nodelocal.go",
         "quit.go",
         "sql.go",
-        "sql_format_table.go",
-        "sql_util.go",
+        "sql_client.go",
         "sqlfmt.go",
         "start.go",
         "start_jemalloc.go",
@@ -97,6 +97,7 @@ go_library(
         "//pkg/build",
         "//pkg/ccl/sqlproxyccl",
         "//pkg/ccl/sqlproxyccl/tenantdirsvr",
+        "//pkg/cli/clierror",
         "//pkg/cli/cliflags",
         "//pkg/cli/clisqlclient",
         "//pkg/cli/exit",
@@ -159,9 +160,7 @@ go_library(
         "//pkg/util",
         "//pkg/util/cgroups",
         "//pkg/util/contextutil",
-        "//pkg/util/duration",
         "//pkg/util/encoding",
-        "//pkg/util/encoding/csv",
         "//pkg/util/envutil",
         "//pkg/util/flagutil",
         "//pkg/util/grpcutil",
@@ -187,7 +186,6 @@ go_library(
         "//pkg/util/timeutil",
         "//pkg/util/tracing",
         "//pkg/util/uuid",
-        "//pkg/util/version",
         "//pkg/workload",
         "//pkg/workload/bank",
         "//pkg/workload/bulkingest",
@@ -201,7 +199,6 @@ go_library(
         "//pkg/workload/workloadsql",
         "//pkg/workload/ycsb",
         "@com_github_cockroachdb_apd_v2//:apd",
-        "@com_github_cockroachdb_cockroach_go//crdb",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_errors//oserror",
         "@com_github_cockroachdb_logtags//:logtags",
@@ -215,10 +212,8 @@ go_library(
         "@com_github_knz_go_libedit//:go-libedit",
         "@com_github_kr_pretty//:pretty",
         "@com_github_lib_pq//:pq",
-        "@com_github_lib_pq_auth_kerberos//:kerberos",
         "@com_github_marusama_semaphore//:semaphore",
         "@com_github_mattn_go_isatty//:go-isatty",
-        "@com_github_olekukonko_tablewriter//:tablewriter",
         "@com_github_spf13_cobra//:cobra",
         "@com_github_spf13_cobra//doc",
         "@com_github_spf13_pflag//:pflag",
@@ -301,9 +296,7 @@ go_test(
         "node_test.go",
         "nodelocal_test.go",
         "quit_test.go",
-        "sql_format_table_test.go",
         "sql_test.go",
-        "sql_util_test.go",
         "sqlfmt_test.go",
         "start_test.go",
         "statement_diag_test.go",
@@ -317,7 +310,9 @@ go_test(
     deps = [
         "//pkg/base",
         "//pkg/build",
+        "//pkg/cli/clierror",
         "//pkg/cli/cliflags",
+        "//pkg/cli/clisqlclient",
         "//pkg/cli/exit",
         "//pkg/gossip",
         "//pkg/gossip/resolver",
@@ -339,12 +334,8 @@ go_test(
         "//pkg/settings/cluster",
         "//pkg/sql",
         "//pkg/sql/catalog/descpb",
-        "//pkg/sql/lex",
         "//pkg/sql/parser",
-        "//pkg/sql/pgwire/pgcode",
-        "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/protoreflect",
-        "//pkg/sql/sem/tree",
         "//pkg/storage",
         "//pkg/testutils",
         "//pkg/testutils/buildutil",
@@ -366,7 +357,6 @@ go_test(
         "@com_github_cockroachdb_datadriven//:datadriven",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_pebble//:pebble",
-        "@com_github_lib_pq//:pq",
         "@com_github_spf13_cobra//:cobra",
         "@com_github_spf13_pflag//:pflag",
         "@com_github_stretchr_testify//assert",

diff --git a/pkg/cli/auth.go b/pkg/cli/auth.go
@@ -16,6 +16,7 @@ import (
 	"net/http"
 	"os"
 
+	"github.com/cockroachdb/cockroach/pkg/cli/clisqlclient"
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
@@ -66,7 +67,7 @@ func runLogin(cmd *cobra.Command, args []string) error {
 		rows := [][]string{
 			{username, fmt.Sprintf("%d", id), hC},
 		}
-		if err := PrintQueryOutput(os.Stdout, cols, NewRowSliceIter(rows, "ll")); err != nil {
+		if err := PrintQueryOutput(os.Stdout, cols, clisqlclient.NewRowSliceIter(rows, "ll")); err != nil {
 			return err
 		}
 
@@ -86,16 +87,18 @@ func runLogin(cmd *cobra.Command, args []string) error {
 	return nil
 }
 
-func createAuthSessionToken(username string) (sessionID int64, httpCookie *http.Cookie, err error) {
+func createAuthSessionToken(
+	username string,
+) (sessionID int64, httpCookie *http.Cookie, resErr error) {
 	sqlConn, err := makeSQLClient("cockroach auth-session login", useSystemDb)
 	if err != nil {
 		return -1, nil, err
 	}
-	defer sqlConn.Close()
+	defer func() { resErr = errors.CombineErrors(resErr, sqlConn.Close()) }()
 
 	// First things first. Does the user exist?
-	_, rows, err := runQuery(sqlConn,
-		makeQuery(`SELECT count(username) FROM system.users WHERE username = $1 AND NOT "isRole"`, username), false)
+	_, rows, err := clisqlclient.RunQuery(sqlConn,
+		clisqlclient.MakeQuery(`SELECT count(username) FROM system.users WHERE username = $1 AND NOT "isRole"`, username), false)
 	if err != nil {
 		return -1, nil, err
 	}
@@ -155,16 +158,16 @@ The user for which the HTTP sessions are revoked can be arbitrary.
 	RunE: MaybeDecorateGRPCError(runLogout),
 }
 
-func runLogout(cmd *cobra.Command, args []string) error {
+func runLogout(cmd *cobra.Command, args []string) (resErr error) {
 	username := tree.Name(args[0]).Normalize()
 
 	sqlConn, err := makeSQLClient("cockroach auth-session logout", useSystemDb)
 	if err != nil {
 		return err
 	}
-	defer sqlConn.Close()
+	defer func() { resErr = errors.CombineErrors(resErr, sqlConn.Close()) }()
 
-	logoutQuery := makeQuery(
+	logoutQuery := clisqlclient.MakeQuery(
 		`UPDATE system.web_sessions SET "revokedAt" = if("revokedAt"::timestamptz<now(),"revokedAt",now())
       WHERE username = $1
   RETURNING username,
@@ -186,14 +189,14 @@ The user invoking the 'list' CLI command must be an admin on the cluster.
 	RunE: MaybeDecorateGRPCError(runAuthList),
 }
 
-func runAuthList(cmd *cobra.Command, args []string) error {
+func runAuthList(cmd *cobra.Command, args []string) (resErr error) {
 	sqlConn, err := makeSQLClient("cockroach auth-session list", useSystemDb)
 	if err != nil {
 		return err
 	}
-	defer sqlConn.Close()
+	defer func() { resErr = errors.CombineErrors(resErr, sqlConn.Close()) }()
 
-	logoutQuery := makeQuery(`
+	logoutQuery := clisqlclient.MakeQuery(`
 SELECT username,
        id AS "session ID",
        "createdAt" as "created",

diff --git a/pkg/cli/binding.go b/pkg/cli/binding.go
@@ -0,0 +1,59 @@
+// Copyright 2021 The Cockroach Authors.
+//
+// Use of this software is governed by the Business Source License
+// included in the file licenses/BSL.txt.
+//
+// As of the Change Date specified in that file, in accordance with
+// the Business Source License, use of this software will be governed
+// by the Apache License, Version 2.0, included in the file
+// licenses/APL.txt.
+
+package cli
+
+import (
+	"io"
+
+	"github.com/cockroachdb/cockroach/pkg/cli/clisqlclient"
+)
+
+// makeSQLConn creates a connection object from
+// a connection URL.
+//
+// This is the local variant of the function in the clisqlclient
+// package.
+func makeSQLConn(url string) clisqlclient.Conn {
+	return clisqlclient.MakeSQLConn(url,
+		cliCtx.isInteractive,
+		&sqlCtx.enableServerExecutionTimings,
+		sqlCtx.embeddedMode,
+		&sqlCtx.echo,
+		&sqlCtx.showTimes,
+		&sqlCtx.verboseTimings,
+	)
+}
+
+// PrintQueryOutput takes a list of column names and a list of row
+// contents writes a formatted table to 'w'.
+//
+// This is the local variant of the function in the clisqlclient
+// package.
+func PrintQueryOutput(w io.Writer, cols []string, allRows clisqlclient.RowStrIter) error {
+	return clisqlclient.PrintQueryOutput(w, cols, allRows,
+		cliCtx.tableDisplayFormat,
+		cliCtx.tableBorderMode,
+	)
+}
+
+// runQueryAndFormatResults takes a 'query' with optional 'parameters'.
+// It runs the sql query and writes output to 'w'.
+//
+// This is the local variant of the function in the clisqlclient
+// package.
+func runQueryAndFormatResults(
+	conn clisqlclient.Conn, w io.Writer, fn clisqlclient.QueryFn,
+) (err error) {
+	return clisqlclient.RunQueryAndFormatResults(conn, w, fn,
+		cliCtx.tableDisplayFormat,
+		cliCtx.tableBorderMode,
+	)
+}
diff --git a/pkg/cli/cert.go b/pkg/cli/cert.go
@@ -16,6 +16,7 @@ import (
 	"strings"
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/cli/clisqlclient"
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/errors"
 	"github.com/spf13/cobra"
@@ -299,7 +300,7 @@ func runListCerts(cmd *cobra.Command, args []string) error {
 		addRow(cert, fmt.Sprintf("user: %s", user))
 	}
 
-	return PrintQueryOutput(os.Stdout, certTableHeaders, NewRowSliceIter(rows, alignment))
+	return PrintQueryOutput(os.Stdout, certTableHeaders, clisqlclient.NewRowSliceIter(rows, alignment))
 }
 
 var certCmds = []*cobra.Command{