bring back error coloring (GoogleContainerTools#5718)

* bring back error coloring

* more descriptive error

* treat tput error as not supporting colors

* move string into constants

* address readability feedback

* Update cmd/skaffold/skaffold.go

Co-authored-by: Brian de Alwis <bsd@acm.org>

* log error at debug level

Co-authored-by: Brian de Alwis <bsd@acm.org>

cmd/skaffold/app/cmd/cmd.go

@@ -56,7 +56,7 @@ const (
 	HouseKeepingMessagesAllowedAnnotation = "skaffold_annotation_housekeeping_allowed"
 )
 
-func NewSkaffoldCommand(out, err io.Writer) *cobra.Command {
+func NewSkaffoldCommand(out, errOut io.Writer) *cobra.Command {
 	updateMsg := make(chan string, 1)
 	surveyPrompt := make(chan bool, 1)
 	metricsPrompt := make(chan bool, 1)
@@ -75,8 +75,7 @@ func NewSkaffoldCommand(out, err io.Writer) *cobra.Command {
 
 			opts.Command = cmd.Use
 			instrumentation.SetCommand(cmd.Use)
-
-			out = color.SetupColors(out, defaultColor, forceColors)
+			out := color.SetupColors(out, defaultColor, forceColors)
 			if timestamps {
 				l := logrus.New()
 				l.SetOutput(out)
@@ -88,7 +87,7 @@ func NewSkaffoldCommand(out, err io.Writer) *cobra.Command {
 			cmd.Root().SetOutput(out)
 
 			// Setup logs
-			if err := setUpLogs(err, v, timestamps); err != nil {
+			if err := setUpLogs(errOut, v, timestamps); err != nil {
 				return err
 			}
 

cmd/skaffold/skaffold.go

@@ -38,7 +38,11 @@ func main() {
 		if errors.Is(err, context.Canceled) {
 			logrus.Debugln("ignore error since context is cancelled:", err)
 		} else {
-			color.Red.Fprintln(os.Stderr, err)
+			// As we allow some color setup using CLI flags for the main run, we can't run SetupColors()
+			// for the entire skaffold run here. It's possible SetupColors() was never called, so call it again
+			// before we print an error to get the right coloring.
+			errOut := color.SetupColors(os.Stderr, color.DefaultColorCode, false)
+			color.Red.Fprintln(errOut, err)
 			code = exitCode(err)
 		}
 	}

pkg/skaffold/color/formatter.go

@@ -24,6 +24,7 @@ import (
 
 	colors "github.com/heroku/color"
 	"github.com/mattn/go-colorable"
+	"github.com/sirupsen/logrus"
 
 	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/util"
 )
@@ -39,7 +40,12 @@ func init() {
 // SetupColors conditionally wraps the input `Writer` with a color enabled `Writer`.
 func SetupColors(out io.Writer, defaultColor int, forceColors bool) io.Writer {
 	_, isTerm := util.IsTerminal(out)
-	useColors := isTerm || forceColors
+	supportsColor, err := util.SupportsColor()
+	if err != nil {
+		logrus.Debugf("error checking for color support: %v", err)
+	}
+
+	useColors := (isTerm && supportsColor) || forceColors
 	if useColors {
 		// Use EnableColorsStdout to enable use of color on Windows
 		useColors = false // value is updated if color-enablement is successful

pkg/skaffold/constants/constants.go

@@ -70,6 +70,8 @@ const (
 	LeeroyAppResponse = "leeroooooy app!!\n"
 
 	GithubIssueLink = "https://github.com/GoogleContainerTools/skaffold/issues/new"
+
+	Windows = "windows"
 )
 
 type Phase string

pkg/skaffold/util/tar.go

@@ -27,6 +27,8 @@ import (
 	"time"
 
 	"github.com/sirupsen/logrus"
+
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 )
 
 type headerModifier func(*tar.Header)
@@ -145,7 +147,7 @@ func addFileToTar(root string, src string, dst string, tw *tar.Writer, hm header
 	}
 
 	// Code copied from https://github.com/moby/moby/blob/master/pkg/archive/archive_windows.go
-	if runtime.GOOS == "windows" {
+	if runtime.GOOS == constants.Windows {
 		header.Mode = int64(chmodTarEntry(os.FileMode(header.Mode)))
 	}
 	if hm != nil {

pkg/skaffold/util/tar_test.go

@@ -26,6 +26,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
@@ -234,7 +235,7 @@ func TestCreateTarWithAbsolutePaths(t *testing.T) {
 }
 
 func TestAddFileToTarSymlinks(t *testing.T) {
-	if runtime.GOOS == "windows" {
+	if runtime.GOOS == constants.Windows {
 		t.Skip("creating symlinks requires extra privileges on Windows")
 	}
 

pkg/skaffold/util/term.go

@@ -17,9 +17,16 @@ limitations under the License.
 package util
 
 import (
+	"fmt"
 	"io"
+	"os/exec"
+	"runtime"
+	"strconv"
+	"strings"
 
 	"golang.org/x/term"
+
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 )
 
 func IsTerminal(w io.Writer) (uintptr, bool) {
@@ -35,3 +42,22 @@ func IsTerminal(w io.Writer) (uintptr, bool) {
 
 	return 0, false
 }
+
+func SupportsColor() (bool, error) {
+	if runtime.GOOS == constants.Windows {
+		return true, nil
+	}
+
+	cmd := exec.Command("tput", "colors")
+	res, err := RunCmdOut(cmd)
+	if err != nil {
+		return false, fmt.Errorf("checking terminal colors: %w", err)
+	}
+
+	numColors, err := strconv.Atoi(strings.TrimSpace(string(res)))
+	if err != nil {
+		return false, err
+	}
+
+	return numColors > 0, nil
+}

pkg/skaffold/util/term_test.go

@@ -18,8 +18,11 @@ package util
 
 import (
 	"bytes"
+	"errors"
+	"runtime"
 	"testing"
 
+	"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
 	"github.com/GoogleContainerTools/skaffold/testutil"
 )
 
@@ -31,3 +34,51 @@ func TestIsNotTerminal(t *testing.T) {
 	testutil.CheckDeepEqual(t, uintptr(0x00), termFd)
 	testutil.CheckDeepEqual(t, false, isTerm)
 }
+
+func TestSupportsColor(t *testing.T) {
+	tests := []struct {
+		description  string
+		colorsOutput string
+		shouldErr    bool
+		expected     bool
+	}{
+		{
+			description:  "Supports 256 colors",
+			colorsOutput: "256",
+			expected:     true,
+		},
+		{
+			description:  "Supports 0 colors",
+			colorsOutput: "0",
+			expected:     false,
+		},
+		{
+			description:  "tput returns -1",
+			colorsOutput: "-1",
+			expected:     false,
+		},
+		{
+			description:  "cmd run errors",
+			colorsOutput: "-1",
+			expected:     false,
+			shouldErr:    true,
+		},
+	}
+
+	for _, test := range tests {
+		testutil.Run(t, test.description, func(t *testutil.T) {
+			if test.shouldErr {
+				t.Override(&DefaultExecCommand, testutil.CmdRunOutErr("tput colors", test.colorsOutput, errors.New("error")))
+			} else {
+				t.Override(&DefaultExecCommand, testutil.CmdRunOut("tput colors", test.colorsOutput))
+			}
+			if runtime.GOOS == constants.Windows {
+				test.expected = true
+				test.shouldErr = false
+			}
+
+			supportsColors, err := SupportsColor()
+			t.CheckErrorAndDeepEqual(test.shouldErr, err, test.expected, supportsColors)
+		})
+	}
+}