Merge pull request #21462 from vespa-engine/mpolden/cli-tweaks

Vespa CLI tweaks

9 files changed, 68 insertions, 110 deletions

diff --git a/client/go/auth0/auth0.go b/client/go/auth0/auth0.go
index bcd70c4f54a..2f7040b3d37 100644
--- a/client/go/auth0/auth0.go
+++ b/client/go/auth0/auth0.go
@@ -386,7 +386,7 @@ func RunLogin(ctx context.Context, a *Auth0, expired bool) (*System, error) {
 	}
 
 	var res auth.Result
-	err = util.Spinner("Waiting for login to complete in browser ...", func() error {
+	err = util.Spinner(os.Stderr, "Waiting for login to complete in browser ...", func() error {
 		res, err = a.Authenticator.Wait(ctx, state)
 		return err
 	})
diff --git a/client/go/cmd/api_key.go b/client/go/cmd/api_key.go
index ae3f5346f4c..438bf017a54 100644
--- a/client/go/cmd/api_key.go
+++ b/client/go/cmd/api_key.go
@@ -57,6 +57,7 @@ var apiKeyCmd = &cobra.Command{
 	RunE:              doApiKey,
 }
 
+// TODO: Remove this after 2022-06-01
 var deprecatedApiKeyCmd = &cobra.Command{
 	Use:               "api-key",
 	Short:             "Create a new user API key for authentication with Vespa Cloud",
diff --git a/client/go/cmd/cert.go b/client/go/cmd/cert.go
index 2a28589aabb..5120459f7ac 100644
--- a/client/go/cmd/cert.go
+++ b/client/go/cmd/cert.go
@@ -22,7 +22,7 @@ var (
 
 func init() {
 	certCmd.Flags().BoolVarP(&overwriteCertificate, "force", "f", false, "Force overwrite of existing certificate and private key")
-	certCmd.Flags().BoolVarP(&noApplicationPackage, "no-add", "N", false, "Do not add certificate to an application package")
+	certCmd.Flags().BoolVarP(&noApplicationPackage, "no-add", "N", false, "Do not add certificate to the application package")
 	certCmd.MarkPersistentFlagRequired(applicationFlag)
 
 	deprecatedCertCmd.Flags().BoolVarP(&overwriteCertificate, "force", "f", false, "Force overwrite of existing certificate and private key")
@@ -33,36 +33,34 @@ func init() {
 	certAddCmd.MarkPersistentFlagRequired(applicationFlag)
 }
 
-var certCmd = &cobra.Command{
-	Use:   "cert",
-	Short: "Create a new private key and self-signed certificate for Vespa Cloud deployment",
-	Long: `Create a new private key and self-signed certificate for Vespa Cloud deployment.
+var longDoc = `Create a new private key and self-signed certificate for Vespa Cloud deployment.
 
 The private key and certificate will be stored in the Vespa CLI home directory
 (see 'vespa help config'). Other commands will then automatically load the
-certificate as necessary.
-
-The certificate will be added to your application package specified as an
-argument to this command (default '.'), unless the '--no-add' is set.
+certificate as necessary. The certificate will be added to your application
+package specified as an argument to this command (default '.').
 
 It's possible to override the private key and certificate used through
 environment variables. This can be useful in continuous integration systems.
 
-* VESPA_CLI_DATA_PLANE_CERT and VESPA_CLI_DATA_PLANE_KEY containing the
-  certificate and private key directly:
+Example of setting the certificate and key in-line:
 
-  export VESPA_CLI_DATA_PLANE_CERT="my cert"
-  export VESPA_CLI_DATA_PLANE_KEY="my private key"
+- export VESPA_CLI_DATA_PLANE_CERT="my cert"
+- export VESPA_CLI_DATA_PLANE_KEY="my private key"
 
-* VESPA_CLI_DATA_PLANE_CERT_FILE and VESPA_CLI_DATA_PLANE_KEY_FILE containing
-  paths to the certificate and private key:
+Example of loading certificate and key from custom paths:
 
-  export VESPA_CLI_DATA_PLANE_CERT_FILE=/path/to/cert
-  export VESPA_CLI_DATA_PLANE_KEY_FILE=/path/to/key
+- export VESPA_CLI_DATA_PLANE_CERT_FILE=/path/to/cert
+- export VESPA_CLI_DATA_PLANE_KEY_FILE=/path/to/key
 
 Note that when overriding key pair through environment variables, that key pair
 will always be used for all applications. It's not possible to specify an
-application-specific key.`,
+application-specific key.`
+
+var certCmd = &cobra.Command{
+	Use:   "cert",
+	Short: "Create a new private key and self-signed certificate for Vespa Cloud deployment",
+	Long:  longDoc,
 	Example: `$ vespa auth cert -a my-tenant.my-app.my-instance
 $ vespa auth cert -a my-tenant.my-app.my-instance path/to/application/package`,
 	DisableAutoGenTag: true,
@@ -71,36 +69,11 @@ $ vespa auth cert -a my-tenant.my-app.my-instance path/to/application/package`,
 	RunE:              doCert,
 }
 
+// TODO: Remove this after 2022-06-01
 var deprecatedCertCmd = &cobra.Command{
-	Use:   "cert",
-	Short: "Create a new private key and self-signed certificate for Vespa Cloud deployment",
-	Long: `Create a new private key and self-signed certificate for Vespa Cloud deployment.
-
-The private key and certificate will be stored in the Vespa CLI home directory
-(see 'vespa help config'). Other commands will then automatically load the
-certificate as necessary.
-
-The certificate will be added to your application package specified as an
-argument to this command (default '.').
-
-It's possible to override the private key and certificate used through
-environment variables. This can be useful in continuous integration systems.
-
-* VESPA_CLI_DATA_PLANE_CERT and VESPA_CLI_DATA_PLANE_KEY containing the
-  certificate and private key directly:
-
-  export VESPA_CLI_DATA_PLANE_CERT="my cert"
-  export VESPA_CLI_DATA_PLANE_KEY="my private key"
-
-* VESPA_CLI_DATA_PLANE_CERT_FILE and VESPA_CLI_DATA_PLANE_KEY_FILE containing
-  paths to the certificate and private key:
-
-  export VESPA_CLI_DATA_PLANE_CERT_FILE=/path/to/cert
-  export VESPA_CLI_DATA_PLANE_KEY_FILE=/path/to/key
-
-Note that when overriding key pair through environment variables, that key pair
-will always be used for all applications. It's not possible to specify an
-application-specific key.`,
+	Use:               "cert",
+	Short:             "Create a new private key and self-signed certificate for Vespa Cloud deployment",
+	Long:              longDoc,
 	Example:           "$ vespa cert -a my-tenant.my-app.my-instance",
 	DisableAutoGenTag: true,
 	SilenceUsage:      true,
@@ -115,7 +88,7 @@ var certAddCmd = &cobra.Command{
 	Short: "Add certificate to application package",
 	Long: `Add an existing self-signed certificate for Vespa Cloud deployment to your application package.
 
-The certificate will be looked for in the Vespa CLI home directory (see 'vespa
+The certificate will be loaded from the Vespa CLI home directory (see 'vespa
 help config') by default.
 
 The location of the application package can be specified as an argument to this
diff --git a/client/go/cmd/clone.go b/client/go/cmd/clone.go
index c0d9ef0cbac..1329e868a9c 100644
--- a/client/go/cmd/clone.go
+++ b/client/go/cmd/clone.go
@@ -120,7 +120,7 @@ func fetchSampleAppsZip(destination string) error {
 		return fmt.Errorf("could not create temporary file: %w", err)
 	}
 	defer f.Close()
-	return util.Spinner(color.Yellow("Downloading sample apps ...").String(), func() error {
+	return util.Spinner(stderr, color.Yellow("Downloading sample apps ...").String(), func() error {
 		request, err := http.NewRequest("GET", "https://github.com/vespa-engine/sample-apps/archive/refs/heads/master.zip", nil)
 		if err != nil {
 			return fmt.Errorf("invalid url: %w", err)
diff --git a/client/go/cmd/deploy.go b/client/go/cmd/deploy.go
index 13f37fa3901..4a7fae243a0 100644
--- a/client/go/cmd/deploy.go
+++ b/client/go/cmd/deploy.go
@@ -9,6 +9,7 @@ import (
 	"log"
 
 	"github.com/spf13/cobra"
+	"github.com/vespa-engine/vespa/client/go/util"
 	"github.com/vespa-engine/vespa/client/go/vespa"
 )
 
@@ -68,7 +69,12 @@ $ vespa deploy -t cloud -z perf.aws-us-east-1c`,
 		if err != nil {
 			return err
 		}
-		sessionOrRunID, err := vespa.Deploy(opts)
+
+		var sessionOrRunID int64
+		err = util.Spinner(stderr, "Uploading application package ...", func() error {
+			sessionOrRunID, err = vespa.Deploy(opts)
+			return err
+		})
 		if err != nil {
 			return err
 		}
@@ -110,9 +116,13 @@ var prepareCmd = &cobra.Command{
 		if err != nil {
 			return err
 		}
-		sessionID, err := vespa.Prepare(vespa.DeploymentOptions{
-			ApplicationPackage: pkg,
-			Target:             target,
+		var sessionID int64
+		err = util.Spinner(stderr, "Uploading application package ...", func() error {
+			sessionID, err = vespa.Prepare(vespa.DeploymentOptions{
+				ApplicationPackage: pkg,
+				Target:             target,
+			})
+			return err
 		})
 		if err != nil {
 			return err
diff --git a/client/go/cmd/root.go b/client/go/cmd/root.go
index cbcbb6e5d12..96521737ba9 100644
--- a/client/go/cmd/root.go
+++ b/client/go/cmd/root.go
@@ -100,7 +100,8 @@ func configureOutput() error {
 	colorize := false
 	switch colorValue {
 	case "auto":
-		colorize = isTerminal()
+		_, nocolor := os.LookupEnv("NO_COLOR") // https://no-color.org
+		colorize = !nocolor && isTerminal()
 	case "always":
 		colorize = true
 	case "never":
diff --git a/client/go/go.mod b/client/go/go.mod
index 08851a01f28..263114fd517 100644
--- a/client/go/go.mod
+++ b/client/go/go.mod
@@ -11,7 +11,6 @@ require (
 	github.com/mattn/go-colorable v0.1.8
 	github.com/mattn/go-isatty v0.0.13
 	github.com/pkg/browser v0.0.0-20210706143420-7d21f8c997e2
-	github.com/pkg/errors v0.9.1
 	github.com/spf13/cobra v1.2.1
 	github.com/spf13/pflag v1.0.5
 	github.com/spf13/viper v1.8.1
diff --git a/client/go/util/spinner.go b/client/go/util/spinner.go
index 9f3c2cb4e44..bdce2dbfabe 100644
--- a/client/go/util/spinner.go
+++ b/client/go/util/spinner.go
@@ -3,64 +3,42 @@
 package util
 
 import (
+	"io"
 	"os"
+	"strings"
 	"time"
 
 	"github.com/briandowns/spinner"
-	"github.com/pkg/errors"
+	"github.com/mattn/go-isatty"
 )
 
-const (
-	spinnerTextDone   = "done"
-	spinnerTextFailed = "failed"
-	spinnerColor      = "blue"
-)
-
-var messages = os.Stderr
-
-func Spinner(text string, fn func() error) error {
-	initialMsg := text + " "
-	doneMsg := "\r" + initialMsg + spinnerTextDone + "\n"
-	failMsg := "\r" + initialMsg + spinnerTextFailed + "\n"
-	return loading(initialMsg, doneMsg, failMsg, fn)
-}
-
-func Waiting(fn func() error) error {
-	return loading("", "", "", fn)
-}
-
-func loading(initialMsg, doneMsg, failMsg string, fn func() error) error {
-	done := make(chan struct{})
-	errc := make(chan error)
-	go func() {
-		defer close(done)
-
-		s := spinner.New(spinner.CharSets[11], 100*time.Millisecond, spinner.WithWriter(messages))
-		s.Prefix = initialMsg
-		s.FinalMSG = doneMsg
-		s.HideCursor = true
-		s.Writer = messages
-
-		if err := s.Color(spinnerColor, "bold"); err != nil {
-			panic(Error(err, "failed setting spinner color"))
-		}
-
+// Spinner writes message to writer w and executes function fn. While fn is running a spinning animation will be
+// displayed after message.
+func Spinner(w io.Writer, message string, fn func() error) error {
+	s := spinner.New(spinner.CharSets[11], 100*time.Millisecond, spinner.WithWriter(w))
+	if err := s.Color("blue", "bold"); err != nil {
+		return err
+	}
+	if !strings.HasSuffix(message, " ") {
+		message += " "
+	}
+	s.Prefix = message
+	s.FinalMSG = "\r" + message + "done\n"
+	isTerminal := isTerminal(w)
+	if isTerminal { // spinner package does this check too, but it's hardcoded to check os.Stdout :(
 		s.Start()
-		err := <-errc
-		if err != nil {
-			s.FinalMSG = failMsg
-		}
-
-		s.Stop()
-	}()
-
+	}
 	err := fn()
-	errc <- err
-	<-done
-
+	if isTerminal {
+		s.Stop()
+	}
+	if err != nil {
+		s.FinalMSG = "\r" + message + "failed\n"
+	}
 	return err
 }
 
-func Error(e error, message string) error {
-	return errors.Wrap(e, message)
+func isTerminal(w io.Writer) bool {
+	f, ok := w.(*os.File)
+	return ok && isatty.IsTerminal(f.Fd())
 }
diff --git a/client/go/vespa/deploy.go b/client/go/vespa/deploy.go
index 3316dcac924..fdc083631f7 100644
--- a/client/go/vespa/deploy.go
+++ b/client/go/vespa/deploy.go
@@ -376,11 +376,7 @@ func uploadApplicationPackage(url *url.URL, opts DeploymentOptions) (int64, erro
 	if err := opts.Target.SignRequest(request, keyID); err != nil {
 		return 0, err
 	}
-	var response *http.Response
-	err = util.Spinner("Uploading application package ...", func() error {
-		response, err = service.Do(request, time.Minute*10)
-		return err
-	})
+	response, err := service.Do(request, time.Minute*10)
 	if err != nil {
 		return 0, err
 	}