fix(text): consistent formatting and output alignment (#1030)

* tests(text): add tests for existing behaviour * fix(text): remove leading linebreak from prefix methods * feat(text): add BreakN * fix(text): prefix methods should have two linebreaks * test(text): assert leading linebreaks are removed * doc(text): document linebreak removal logic * Rewrite this commit message once everything is fixed * feat: add text.ParseBreaks() * fix: tests * fix: manual checks * fix: init test * fix: more manual testing * fix(compute/deploy): add missing spinner.StopFail
fastly · Oct 5, 2023 · 05f3c00 · 05f3c00
1 parent 832165d
commit 05f3c00
Show file tree

Hide file tree

Showing 65 changed files with 535 additions and 313 deletions.
diff --git a/cmd/fastly/main.go b/cmd/fastly/main.go
@@ -17,6 +17,7 @@ import (
 	"github.com/fastly/cli/pkg/github"
 	"github.com/fastly/cli/pkg/manifest"
 	"github.com/fastly/cli/pkg/sync"
+	"github.com/fastly/cli/pkg/text"
 )
 
 func main() {
@@ -125,15 +126,14 @@ func main() {
 	}
 
 	if err != nil {
+		text.Break(out)
 		fsterr.Deduce(err).Print(color.Error)
-
 		exitError := fsterr.SkipExitError{}
 		if errors.As(err, &exitError) {
 			if exitError.Skip {
 				return // skip returning an error for 'help' output
 			}
 		}
-
 		os.Exit(1)
 	}
 }

diff --git a/pkg/app/run.go b/pkg/app/run.go
@@ -135,10 +135,9 @@ func Run(opts RunOpts) error {
 		if fi, err := os.Stat(config.FilePath); err == nil {
 			if mode := fi.Mode().Perm(); mode > config.FilePermissions {
 				if !g.Flags.Quiet {
-					text.Warning(opts.Stdout, "Unprotected configuration file.")
-					fmt.Fprintf(opts.Stdout, "Permissions for '%s' are too open\n", config.FilePath)
-					fmt.Fprintf(opts.Stdout, "It is recommended that your configuration file is NOT accessible by others.\n")
-					fmt.Fprintln(opts.Stdout)
+					text.Warning(opts.Stdout, "Unprotected configuration file.\n\n")
+					text.Output(opts.Stdout, "Permissions for '%s' are too open\n\n", config.FilePath)
+					text.Output(opts.Stdout, "It is recommended that your configuration file is NOT accessible by others.\n\n")
 				}
 			}
 		}

diff --git a/pkg/cmd/flags.go b/pkg/cmd/flags.go
@@ -233,8 +233,9 @@ func (ac *OptionalAutoClone) Parse(v *fastly.Version, sid string, verbose bool,
 			return nil, fmt.Errorf("error cloning service version: %w", err)
 		}
 		if verbose {
-			msg := fmt.Sprintf("Service version %d is not editable, so it was automatically cloned because --autoclone is enabled. Now operating on version %d.", v.Number, version.Number)
-			text.Output(out, msg)
+			msg := "Service version %d is not editable, so it was automatically cloned because --autoclone is enabled. Now operating on version %d.\n\n"
+			format := fmt.Sprintf(msg, v.Number, version.Number)
+			text.Output(out, format)
 		}
 		return version, nil
 	}

diff --git a/pkg/commands/acl/create.go b/pkg/commands/acl/create.go
@@ -3,12 +3,13 @@ package acl
 import (
 	"io"
 
+	"github.com/fastly/go-fastly/v8/fastly"
+
 	"github.com/fastly/cli/pkg/cmd"
 	"github.com/fastly/cli/pkg/errors"
 	"github.com/fastly/cli/pkg/global"
 	"github.com/fastly/cli/pkg/manifest"
 	"github.com/fastly/cli/pkg/text"
-	"github.com/fastly/go-fastly/v8/fastly"
 )
 
 // NewCreateCommand returns a usable command registered under the parent.
@@ -84,7 +85,6 @@ func (c *CreateCommand) Exec(_ io.Reader, out io.Writer) error {
 	}
 
 	input := c.constructInput(serviceID, serviceVersion.Number)
-
 	a, err := c.Globals.APIClient.CreateACL(input)
 	if err != nil {
 		c.Globals.ErrLog.AddWithContext(err, map[string]any{

diff --git a/pkg/commands/acl/delete.go b/pkg/commands/acl/delete.go
@@ -3,12 +3,13 @@ package acl
 import (
 	"io"
 
+	"github.com/fastly/go-fastly/v8/fastly"
+
 	"github.com/fastly/cli/pkg/cmd"
 	"github.com/fastly/cli/pkg/errors"
 	"github.com/fastly/cli/pkg/global"
 	"github.com/fastly/cli/pkg/manifest"
 	"github.com/fastly/cli/pkg/text"
-	"github.com/fastly/go-fastly/v8/fastly"
 )
 
 // NewDeleteCommand returns a usable command registered under the parent.
@@ -82,7 +83,6 @@ func (c *DeleteCommand) Exec(_ io.Reader, out io.Writer) error {
 	}
 
 	input := c.constructInput(serviceID, serviceVersion.Number)
-
 	err = c.Globals.APIClient.DeleteACL(input)
 	if err != nil {
 		c.Globals.ErrLog.AddWithContext(err, map[string]any{

diff --git a/pkg/commands/aclentry/delete.go b/pkg/commands/aclentry/delete.go
@@ -3,11 +3,12 @@ package aclentry
 import (
 	"io"
 
+	"github.com/fastly/go-fastly/v8/fastly"
+
 	"github.com/fastly/cli/pkg/cmd"
 	"github.com/fastly/cli/pkg/global"
 	"github.com/fastly/cli/pkg/manifest"
 	"github.com/fastly/cli/pkg/text"
-	"github.com/fastly/go-fastly/v8/fastly"
 )
 
 // NewDeleteCommand returns a usable command registered under the parent.
@@ -62,7 +63,6 @@ func (c *DeleteCommand) Exec(_ io.Reader, out io.Writer) error {
 	}
 
 	input := c.constructInput(serviceID)
-
 	err = c.Globals.APIClient.DeleteACLEntry(input)
 	if err != nil {
 		c.Globals.ErrLog.AddWithContext(err, map[string]any{

diff --git a/pkg/commands/authtoken/authtoken_test.go b/pkg/commands/authtoken/authtoken_test.go
@@ -423,7 +423,7 @@ Expires at: 2021-06-15 23:00:00 +0000 UTC
 func listTokenOutputSummary(env bool) string {
 	var msg string
 	if env {
-		msg = "\nINFO: Listing customer tokens for the FASTLY_CUSTOMER_ID environment variable\n\n"
+		msg = "INFO: Listing customer tokens for the FASTLY_CUSTOMER_ID environment variable\n\n"
 	}
 	return fmt.Sprintf(`%sNAME  TOKEN ID  USER ID  SCOPE                  SERVICES
 Foo   123       456      purge_all global:read  a, b

diff --git a/pkg/commands/authtoken/delete.go b/pkg/commands/authtoken/delete.go
@@ -7,13 +7,14 @@ import (
 	"os"
 	"path/filepath"
 
+	"github.com/fastly/go-fastly/v8/fastly"
+
 	"github.com/fastly/cli/pkg/cmd"
 	"github.com/fastly/cli/pkg/errors"
 	"github.com/fastly/cli/pkg/global"
 	"github.com/fastly/cli/pkg/lookup"
 	"github.com/fastly/cli/pkg/manifest"
 	"github.com/fastly/cli/pkg/text"
-	"github.com/fastly/go-fastly/v8/fastly"
 )
 
 // NewDeleteCommand returns a usable command registered under the parent.
@@ -79,6 +80,7 @@ func (c *DeleteCommand) Exec(_ io.Reader, out io.Writer) error {
 
 		text.Success(out, "Deleted tokens")
 		if c.Globals.Verbose() {
+			text.Break(out)
 			c.printTokens(out, input.Tokens)
 		}
 		return nil
@@ -146,7 +148,6 @@ func (c *DeleteCommand) constructInputBatch() (*fastly.BatchDeleteTokensInput, e
 
 // printTokens displays the tokens provided by a user.
 func (c *DeleteCommand) printTokens(out io.Writer, rs []*fastly.BatchToken) {
-	fmt.Fprintf(out, "\n")
 	t := text.NewTable(out)
 	t.AddHeader("TOKEN ID")
 	for _, r := range rs {

diff --git a/pkg/commands/authtoken/list.go b/pkg/commands/authtoken/list.go
@@ -5,13 +5,14 @@ import (
 	"io"
 	"strings"
 
+	"github.com/fastly/go-fastly/v8/fastly"
+
 	"github.com/fastly/cli/pkg/cmd"
 	fsterr "github.com/fastly/cli/pkg/errors"
 	"github.com/fastly/cli/pkg/global"
 	"github.com/fastly/cli/pkg/lookup"
 	"github.com/fastly/cli/pkg/manifest"
 	"github.com/fastly/cli/pkg/text"
-	"github.com/fastly/go-fastly/v8/fastly"
 )
 
 // NewListCommand returns a usable command registered under the parent.
@@ -60,12 +61,9 @@ func (c *ListCommand) Exec(_ io.Reader, out io.Writer) error {
 
 	if err = c.customerID.Parse(); err == nil {
 		if !c.customerID.WasSet && !c.Globals.Flags.Quiet {
-			text.Info(out, "Listing customer tokens for the FASTLY_CUSTOMER_ID environment variable")
-			text.Break(out)
+			text.Info(out, "Listing customer tokens for the FASTLY_CUSTOMER_ID environment variable\n\n")
 		}
-
 		input := c.constructInput()
-
 		o, err = c.Globals.APIClient.ListCustomerTokens(input)
 		if err != nil {
 			c.Globals.ErrLog.Add(err)

diff --git a/pkg/commands/backend/create.go b/pkg/commands/backend/create.go
@@ -4,12 +4,13 @@ import (
 	"io"
 	"net"
 
+	"github.com/fastly/go-fastly/v8/fastly"
+
 	"github.com/fastly/cli/pkg/cmd"
 	"github.com/fastly/cli/pkg/errors"
 	"github.com/fastly/cli/pkg/global"
 	"github.com/fastly/cli/pkg/manifest"
 	"github.com/fastly/cli/pkg/text"
-	"github.com/fastly/go-fastly/v8/fastly"
 )
 
 // CreateCommand calls the Fastly API to create backends.
@@ -208,7 +209,7 @@ func (c *CreateCommand) Exec(_ io.Reader, out io.Writer) error {
 		input.Port = &c.port.Value
 	case c.useSSL.WasSet && c.useSSL.Value:
 		if c.Globals.Flags.Verbose {
-			text.Warning(out, "Use-ssl was set but no port was specified, using default port 443")
+			text.Warning(out, "Use-ssl was set but no port was specified, using default port 443\n\n")
 		}
 		input.Port = fastly.Int(443)
 	}

diff --git a/pkg/commands/compute/build.go b/pkg/commands/compute/build.go
@@ -166,7 +166,7 @@ func (c *BuildCommand) Exec(in io.Reader, out io.Writer) (err error) {
 	}
 
 	out = originalOut
-	text.Success(out, "Built package (%s)", dest)
+	text.Success(out, "\nBuilt package (%s)", dest)
 	return nil
 }
 
@@ -330,7 +330,7 @@ func language(toolchain string, c *BuildCommand, in io.Reader, out io.Writer, sp
 // The directory is required so a main.wasm can be placed inside it.
 func binDir(c *BuildCommand) error {
 	if c.Globals.Verbose() {
-		text.Info(c.Globals.Output, "Creating ./bin directory (for Wasm binary)")
+		text.Info(c.Globals.Output, "\nCreating ./bin directory (for Wasm binary)\n\n")
 	}
 	dir, err := os.Getwd()
 	if err != nil {

diff --git a/pkg/commands/compute/deploy.go b/pkg/commands/compute/deploy.go
@@ -178,20 +178,24 @@ func (c *DeployCommand) Exec(in io.Reader, out io.Writer) (err error) {
 		// Because the service availability can return an error (which we ignore),
 		// then we need to check for the 'no error' scenarios.
 		if err == nil {
-			if validStatusCodeRange(c.StatusCheckCode) && status != c.StatusCheckCode {
+			switch {
+			case validStatusCodeRange(c.StatusCheckCode) && status != c.StatusCheckCode:
 				// If the user set a specific status code expectation...
 				text.Warning(out, "The service path `%s` responded with a status code (%d) that didn't match what was expected (%d).", c.StatusCheckPath, status, c.StatusCheckCode)
-			} else if !validStatusCodeRange(c.StatusCheckCode) && status >= http.StatusBadRequest {
+			case !validStatusCodeRange(c.StatusCheckCode) && status >= http.StatusBadRequest:
 				// If no status code was specified, and the actual status response was an error...
 				text.Info(out, "The service path `%s` responded with a non-successful status code (%d). Please check your application code if this is an unexpected response.", c.StatusCheckPath, status)
+			default:
+				text.Break(out)
 			}
 		}
 	}
 
-	text.Break(out)
+	if !newService {
+		text.Break(out)
+	}
 	text.Description(out, "Manage this service at", fmt.Sprintf("%s%s", manageServiceBaseURL, serviceID))
 	text.Description(out, "View this service at", serviceURL)
-
 	text.Success(out, "Deployed package (service %s, version %v)", serviceID, serviceVersion.Number)
 	return nil
 }
@@ -343,7 +347,7 @@ func readManifestFromPackageArchive(data *manifest.Data, packageFlag string, ver
 	}
 
 	if verbose {
-		text.Info(out, "Using fastly.toml within --package archive:\n\t%s", packageFlag)
+		text.Info(out, "Using fastly.toml within --package archive: %s\n\n", packageFlag)
 	}
 
 	return nil
@@ -504,19 +508,17 @@ func manageNoServiceIDFlow(
 		text.Output(out, "Press ^C at any time to quit.")
 
 		if manifestFile.Setup.Defined() {
-			text.Info(out, "Processing of the fastly.toml [setup] configuration happens only when there is no existing service. Once a service is created, any further changes to the service or its resources must be made manually.")
+			text.Info(out, "\nProcessing of the fastly.toml [setup] configuration happens only when there is no existing service. Once a service is created, any further changes to the service or its resources must be made manually.")
 		}
 
 		text.Break(out)
-
 		answer, err := text.AskYesNo(out, text.BoldYellow("Create new service: [y/N] "), in)
 		if err != nil {
 			return serviceID, serviceVersion, err
 		}
 		if !answer {
 			return serviceID, serviceVersion, nil
 		}
-
 		text.Break(out)
 	}
 
@@ -526,15 +528,16 @@ func manageNoServiceIDFlow(
 	// The service name will be whatever is set in the --service-name flag.
 	// If the flag isn't set, and we're able to prompt, we'll ask the user.
 	// If the flag isn't set, and we're non-interactive, we'll use the default.
-	if serviceNameFlag.WasSet {
+	switch {
+	case serviceNameFlag.WasSet:
 		serviceName = serviceNameFlag.Value
-	} else if !f.AcceptDefaults && !f.NonInteractive {
+	case f.AcceptDefaults || f.NonInteractive:
+		serviceName = defaultServiceName
+	default:
 		serviceName, err = text.Input(out, text.BoldYellow(fmt.Sprintf("Service name: [%s] ", defaultServiceName)), in)
 		if err != nil || serviceName == "" {
 			serviceName = defaultServiceName
 		}
-	} else {
-		serviceName = defaultServiceName
 	}
 
 	// There is no service and so we'll do a one time creation of the service
@@ -641,6 +644,12 @@ func createService(
 			return createService(f, serviceName, apiClient, fnActivateTrial, spinner, errLog, out)
 		}
 
+		spinner.StopFailMessage(msg)
+		spinErr := spinner.StopFail()
+		if spinErr != nil {
+			return "", nil, spinErr
+		}
+
 		errLog.AddWithContext(err, map[string]any{
 			"Service Name": serviceName,
 		})
@@ -659,7 +668,7 @@ func createService(
 // It deletes the service, which will cause any contained resources to be deleted.
 // It will also strip the Service ID from the fastly.toml manifest file.
 func cleanupService(apiClient api.Interface, serviceID string, m manifest.Data, out io.Writer) error {
-	text.Info(out, "Cleaning up service")
+	text.Info(out, "\nCleaning up service\n\n")
 
 	err := apiClient.DeleteService(&fastly.DeleteServiceInput{
 		ID: serviceID,
@@ -668,7 +677,7 @@ func cleanupService(apiClient api.Interface, serviceID string, m manifest.Data,
 		return err
 	}
 
-	text.Info(out, "Removing Service ID from fastly.toml")
+	text.Info(out, "Removing Service ID from fastly.toml\n\n")
 
 	err = updateManifestServiceID(&m.File, manifest.Filename, "")
 	if err != nil {
@@ -753,10 +762,9 @@ func manageExistingServiceFlow(
 			return serviceVersion, fmt.Errorf("error cloning service version: %w", err)
 		}
 		if verbose {
-			msg := fmt.Sprintf("Service version %d is not editable, so it was automatically cloned. Now operating on version %d.", serviceVersion.Number, clonedVersion.Number)
-			text.Break(out)
-			text.Output(out, msg)
-			text.Break(out)
+			msg := "Service version %d is not editable, so it was automatically cloned. Now operating on version %d.\n\n"
+			format := fmt.Sprintf(msg, serviceVersion.Number, clonedVersion.Number)
+			text.Output(out, format)
 		}
 		serviceVersion = clonedVersion
 	}

diff --git a/pkg/commands/compute/hashsum.go b/pkg/commands/compute/hashsum.go
@@ -40,14 +40,17 @@ func NewHashsumCommand(parent cmd.Registerer, g *global.Data, build *BuildComman
 func (c *HashsumCommand) Exec(in io.Reader, out io.Writer) (err error) {
 	if !c.Globals.Flags.Quiet {
 		text.Warning(out, "This command is deprecated. Use `fastly compute hash-files` instead.")
-		text.Break(out)
+		if c.Globals.Verbose() || c.SkipBuild {
+			text.Break(out)
+		}
 	}
 
 	if !c.SkipBuild {
 		err = c.Build(in, out)
 		if err != nil {
 			return err
 		}
+		text.Break(out)
 	}
 
 	pkgPath, err := validatePackage(c.Manifest, c.Package, c.Globals.Verbose(), c.Globals.ErrLog, out)
@@ -62,10 +65,6 @@ func (c *HashsumCommand) Exec(in io.Reader, out io.Writer) (err error) {
 		}
 	}
 
-	if c.Globals.Verbose() {
-		text.Break(out)
-	}
-
 	hashSum, err := getHashSum(pkgPath)
 	if err != nil {
 		return err