Merge pull request #21654 from vespa-engine/ean/spinner-issues-in-CI

Ean/spinner issues in ci

client/go/auth/auth0/auth0.go

@@ -18,9 +18,7 @@ import (
 	"time"
 
 	"github.com/lestrrat-go/jwx/jwt"
-	"github.com/pkg/browser"
 	"github.com/vespa-engine/vespa/client/go/auth"
-	"github.com/vespa-engine/vespa/client/go/util"
 )
 
 const accessTokenExpThreshold = 5 * time.Minute
@@ -138,9 +136,7 @@ func (a *Auth0) IsLoggedIn() bool {
 }
 
 // PrepareSystem loads the System, refreshing its token if necessary.
-// The System access token needs a refresh if:
-// 1. the System scopes are different from the currently required scopes - (auth0 changes).
-// 2. the access token is expired.
+// The System access token needs a refresh if the access token has expired.
 func (a *Auth0) PrepareSystem(ctx context.Context) (*System, error) {
 	if err := a.init(); err != nil {
 		return nil, err
@@ -150,11 +146,10 @@ func (a *Auth0) PrepareSystem(ctx context.Context) (*System, error) {
 		return nil, err
 	}
 
-	if s.AccessToken == "" || scopesChanged(s) {
-		s, err = RunLogin(ctx, a, true)
-		if err != nil {
-			return nil, err
-		}
+	if s.AccessToken == "" {
+		return nil, fmt.Errorf("access token missing: re-authenticate with 'vespa auth login'")
+	} else if scopesChanged(s) {
+		return nil, fmt.Errorf("authentication scopes cahnges: re-authenticate with 'vespa auth login'")
 	} else if isExpired(s.ExpiresAt, accessTokenExpThreshold) {
 		// check if the stored access token is expired:
 		// use the refresh token to get a new access token:
@@ -257,7 +252,7 @@ func (a *Auth0) AddSystem(s *System) error {
 	return nil
 }
 
-func (a *Auth0) removeSystem(s string) error {
+func (a *Auth0) RemoveSystem(s string) error {
 	_ = a.init()
 
 	// If we're dealing with an empty file, we'll need to initialize this map.
@@ -350,84 +345,3 @@ func (a *Auth0) initContext() (err error) {
 	a.config = *cfg
 	return nil
 }
-
-// RunLogin runs the login flow guiding the user through the process
-// by showing the login instructions, opening the browser.
-// Use `expired` to run the login from other commands setup:
-// this will only affect the messages.
-func RunLogin(ctx context.Context, a *Auth0, expired bool) (*System, error) {
-	if expired {
-		fmt.Println("Please sign in to re-authorize the CLI.")
-	}
-
-	state, err := a.Authenticator.Start(ctx)
-	if err != nil {
-		return nil, fmt.Errorf("could not start the authentication process: %w", err)
-	}
-
-	fmt.Printf("Your Device Confirmation code is: %s\n\n", state.UserCode)
-
-	fmt.Println("If you prefer, you can open the URL directly for verification")
-	fmt.Printf("Your Verification URL: %s\n\n", state.VerificationURI)
-
-	fmt.Println("Press Enter to open the browser to log in or ^C to quit...")
-	fmt.Scanln()
-
-	err = browser.OpenURL(state.VerificationURI)
-
-	if err != nil {
-		fmt.Printf("Couldn't open the URL, please do it manually: %s.", state.VerificationURI)
-	}
-
-	var res auth.Result
-	err = util.Spinner(os.Stderr, "Waiting for login to complete in browser ...", func() error {
-		res, err = a.Authenticator.Wait(ctx, state)
-		return err
-	})
-
-	if err != nil {
-		return nil, fmt.Errorf("login error: %w", err)
-	}
-
-	fmt.Print("\n")
-	fmt.Println("Successfully logged in.")
-	fmt.Print("\n")
-
-	// store the refresh token
-	secretsStore := &auth.Keyring{}
-	err = secretsStore.Set(auth.SecretsNamespace, a.system, res.RefreshToken)
-	if err != nil {
-		// log the error but move on
-		fmt.Println("Could not store the refresh token locally, please expect to login again once your access token expired.")
-	}
-
-	s := System{
-		Name:        a.system,
-		AccessToken: res.AccessToken,
-		ExpiresAt:   time.Now().Add(time.Duration(res.ExpiresIn) * time.Second),
-		Scopes:      auth.RequiredScopes(),
-	}
-	err = a.AddSystem(&s)
-	if err != nil {
-		return nil, fmt.Errorf("could not add system to config: %w", err)
-	}
-
-	return &s, nil
-}
-
-func RunLogout(a *Auth0) error {
-	s, err := a.getSystem()
-	if err != nil {
-		return err
-	}
-
-	if err := a.removeSystem(s.Name); err != nil {
-		return err
-	}
-
-	fmt.Print("\n")
-	fmt.Println("Successfully logged out.")
-	fmt.Print("\n")
-
-	return nil
-}

client/go/cmd/clone.go

@@ -19,7 +19,6 @@ import (
 
 	"github.com/fatih/color"
 	"github.com/spf13/cobra"
-	"github.com/vespa-engine/vespa/client/go/util"
 )
 
 func newCloneCmd(cli *CLI) *cobra.Command {
@@ -116,7 +115,7 @@ func fetchSampleAppsZip(destination string, cli *CLI) error {
 		return fmt.Errorf("could not create temporary file: %w", err)
 	}
 	defer f.Close()
-	return util.Spinner(cli.Stderr, color.YellowString("Downloading sample apps ..."), func() error {
+	return cli.spinner(cli.Stderr, color.YellowString("Downloading sample apps ..."), 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)

client/go/cmd/deploy.go

@@ -12,7 +12,6 @@ import (
 
 	"github.com/fatih/color"
 	"github.com/spf13/cobra"
-	"github.com/vespa-engine/vespa/client/go/util"
 	"github.com/vespa-engine/vespa/client/go/vespa"
 )
 
@@ -54,7 +53,7 @@ $ vespa deploy -t cloud -z perf.aws-us-east-1c`,
 			opts := cli.createDeploymentOptions(pkg, target)
 
 			var result vespa.PrepareResult
-			err = util.Spinner(cli.Stderr, "Uploading application package ...", func() error {
+			err = cli.spinner(cli.Stderr, "Uploading application package ...", func() error {
 				result, err = vespa.Deploy(opts)
 				return err
 			})
@@ -103,7 +102,7 @@ func newPrepareCmd(cli *CLI) *cobra.Command {
 			}
 			opts := cli.createDeploymentOptions(pkg, target)
 			var result vespa.PrepareResult
-			err = util.Spinner(cli.Stderr, "Uploading application package ...", func() error {
+			err = cli.spinner(cli.Stderr, "Uploading application package ...", func() error {
 				result, err = vespa.Prepare(opts)
 				return err
 			})

client/go/cmd/login.go

@@ -1,10 +1,21 @@
 package cmd
 
 import (
+	"fmt"
+	"log"
+	"os"
+	"time"
+
+	"github.com/pkg/browser"
 	"github.com/spf13/cobra"
+	"github.com/vespa-engine/vespa/client/go/auth"
 	"github.com/vespa-engine/vespa/client/go/auth/auth0"
 )
 
+// newLoginCmd runs the login flow guiding the user through the process
+// by showing the login instructions, opening the browser.
+// Use `expired` to run the login from other commands setup:
+// this will only affect the messages.
 func newLoginCmd(cli *CLI) *cobra.Command {
 	return &cobra.Command{
 		Use:               "login",
@@ -27,7 +38,57 @@ func newLoginCmd(cli *CLI) *cobra.Command {
 			if err != nil {
 				return err
 			}
-			_, err = auth0.RunLogin(ctx, a, false)
+			state, err := a.Authenticator.Start(ctx)
+			if err != nil {
+				return fmt.Errorf("could not start the authentication process: %w", err)
+			}
+
+			log.Printf("Your Device Confirmation code is: %s\n\n", state.UserCode)
+
+			log.Println("If you prefer, you can open the URL directly for verification")
+			log.Printf("Your Verification URL: %s\n\n", state.VerificationURI)
+
+			log.Println("Press Enter to open the browser to log in or ^C to quit...")
+			fmt.Scanln()
+
+			err = browser.OpenURL(state.VerificationURI)
+
+			if err != nil {
+				log.Printf("Couldn't open the URL, please do it manually: %s.", state.VerificationURI)
+			}
+
+			var res auth.Result
+			err = cli.spinner(os.Stderr, "Waiting for login to complete in browser ...", func() error {
+				res, err = a.Authenticator.Wait(ctx, state)
+				return err
+			})
+
+			if err != nil {
+				return fmt.Errorf("login error: %w", err)
+			}
+
+			log.Print("\n")
+			log.Println("Successfully logged in.")
+			log.Print("\n")
+
+			// store the refresh token
+			secretsStore := &auth.Keyring{}
+			err = secretsStore.Set(auth.SecretsNamespace, system.Name, res.RefreshToken)
+			if err != nil {
+				// log the error but move on
+				log.Println("Could not store the refresh token locally, please expect to login again once your access token expired.")
+			}
+
+			s := auth0.System{
+				Name:        system.Name,
+				AccessToken: res.AccessToken,
+				ExpiresAt:   time.Now().Add(time.Duration(res.ExpiresIn) * time.Second),
+				Scopes:      auth.RequiredScopes(),
+			}
+			err = a.AddSystem(&s)
+			if err != nil {
+				return fmt.Errorf("could not add system to config: %w", err)
+			}
 			return err
 		},
 	}

client/go/cmd/logout.go

@@ -1,6 +1,8 @@
 package cmd
 
 import (
+	"log"
+
 	"github.com/spf13/cobra"
 	"github.com/vespa-engine/vespa/client/go/auth/auth0"
 )
@@ -26,8 +28,14 @@ func newLogoutCmd(cli *CLI) *cobra.Command {
 			if err != nil {
 				return err
 			}
-			err = auth0.RunLogout(a)
-			return err
+			if err := a.RemoveSystem(system.Name); err != nil {
+				return err
+			}
+
+			log.Print("\n")
+			log.Println("Successfully logged out.")
+			log.Print("\n")
+			return nil
 		},
 	}
 }

client/go/cmd/root.go

@@ -52,6 +52,7 @@ type CLI struct {
 	httpClient util.HTTPClient
 	exec       executor
 	isTerminal func() bool
+	spinner    func(w io.Writer, message string, fn func() error) error
 }
 
 // Flags holds the global Flags of Vespa CLI.
@@ -141,6 +142,7 @@ Vespa documentation: https://docs.vespa.ai`,
 	if err := cli.loadConfig(); err != nil {
 		return nil, err
 	}
+	cli.configureSpinner()
 	cli.configureCommands()
 	cmd.PersistentPreRunE = cli.configureOutput
 	return &cli, nil
@@ -203,6 +205,19 @@ func (c *CLI) configureFlags() {
 	c.flags = &flags
 }
 
+func (c *CLI) configureSpinner() {
+	// Explicitly disable spinner for Screwdriver. It emulates a tty but
+	// \r result in a newline, and output gets truncated.
+	_, screwdriver := c.Environment["SCREWDRIVER"]
+	if c.flags.quiet || !c.isTerminal() || screwdriver {
+		c.spinner = func(w io.Writer, message string, fn func() error) error {
+			return fn()
+		}
+	} else {
+		c.spinner = util.Spinner
+	}
+}
+
 func (c *CLI) configureCommands() {
 	rootCmd := c.cmd
 	authCmd := newAuthCmd()

client/go/util/spinner.go

@@ -4,12 +4,10 @@ package util
 
 import (
 	"io"
-	"os"
 	"strings"
 	"time"
 
 	"github.com/briandowns/spinner"
-	"github.com/mattn/go-isatty"
 )
 
 // Spinner writes message to writer w and executes function fn. While fn is running a spinning animation will be
@@ -24,21 +22,11 @@ func Spinner(w io.Writer, message string, fn func() error) error {
 	}
 	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()
-	}
+	s.Start()
 	err := fn()
-	if isTerminal {
-		s.Stop()
-	}
 	if err != nil {
 		s.FinalMSG = "\r" + message + "failed\n"
 	}
+	s.Stop()
 	return err
 }
-
-func isTerminal(w io.Writer) bool {
-	f, ok := w.(*os.File)
-	return ok && isatty.IsTerminal(f.Fd())
-}