progress: auto-select progress based on if we run on a terminal

This commit adds automatic progress bar selection based on checking
if we run on a terminal or not. When running on a terminal we use
the nice "terminalProgressBar". If that is not set we assuem
we run in a script or CI/CD environment and select plainProgressBar.

Thanks Colin for the hint about the bad integration test.

bib/internal/progress/export_test.go

@@ -17,3 +17,11 @@ func MockOsStderr(w io.Writer) (restore func()) {
 		osStderr = saved
 	}
 }
+
+func MockIsattyIsTerminal(fn func(uintptr) bool) (restore func()) {
+	saved := isattyIsTerminal
+	isattyIsTerminal = fn
+	return func() {
+		isattyIsTerminal = saved
+	}
+}

bib/internal/progress/progress.go

@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"github.com/cheggaaa/pb/v3"
+	"github.com/mattn/go-isatty"
 	"github.com/sirupsen/logrus"
 
 	"github.com/osbuild/images/pkg/osbuild"
@@ -66,12 +67,19 @@ type ProgressBar interface {
 	Stop()
 }
 
+var isattyIsTerminal = isatty.IsTerminal
+
 // New creates a new progressbar based on the requested type
 func New(typ string) (ProgressBar, error) {
 	switch typ {
-	// XXX: autoseelct based on PS1 value (i.e. use term in
-	// interactive shells only?)
-	case "", "plain":
+	case "":
+		// autoselect based on if we are on an interactive
+		// terminal, use plain progress for scripts
+		if isattyIsTerminal(os.Stdin.Fd()) {
+			return NewTerminalProgressBar()
+		}
+		return NewPlainProgressBar()
+	case "plain":
 		return NewPlainProgressBar()
 	case "term":
 		return NewTerminalProgressBar()

bib/internal/progress/progress_test.go

@@ -111,3 +111,22 @@ func TestTermProgress(t *testing.T) {
 	// check shutdown
 	assert.Contains(t, buf.String(), progress.CURSOR_SHOW)
 }
+
+func TestProgressNewAutoselect(t *testing.T) {
+	for _, tc := range []struct {
+		onTerm   bool
+		expected interface{}
+	}{
+		{false, &progress.PlainProgressBar{}},
+		{true, &progress.TerminalProgressBar{}},
+	} {
+		restore := progress.MockIsattyIsTerminal(func(uintptr) bool {
+			return tc.onTerm
+		})
+		defer restore()
+
+		pb, err := progress.New("")
+		assert.NoError(t, err)
+		assert.Equal(t, reflect.TypeOf(pb), reflect.TypeOf(tc.expected), fmt.Sprintf("[%v] %T not the expected %T", tc.onTerm, pb, tc.expected))
+	}
+}

test/test_progress.py

@@ -5,21 +5,18 @@
 from containerbuild import build_container_fixture, build_fake_container_fixture
 
 
-def test_progress_debug(tmp_path, container_storage, build_fake_container):
+def test_progress_debug(tmp_path, build_fake_container):
     output_path = tmp_path / "output"
     output_path.mkdir(exist_ok=True)
 
     cmdline = [
         "podman", "run", "--rm",
         "--privileged",
-        "--security-opt", "label=type:unconfined_t",
-        "-v", f"{container_storage}:/var/lib/containers/storage",
-        "-v", f"{output_path}:/output",
         build_fake_container,
         "build",
+        "--progress=debug",
         "quay.io/centos-bootc/centos-bootc:stream9",
     ]
-    cmdline.append("--progress=debug")
     res = subprocess.run(cmdline, capture_output=True, check=True, text=True)
     assert res.stderr.count("Start progressbar") == 1
     assert res.stderr.count("Manifest generation step") == 1
@@ -29,22 +26,41 @@ def test_progress_debug(tmp_path, container_storage, build_fake_container):
     assert res.stdout.strip() == ""
 
 
-def test_progress_term(tmp_path, container_storage, build_fake_container):
+def test_progress_term_works_without_tty(tmp_path, build_fake_container):
     output_path = tmp_path / "output"
     output_path.mkdir(exist_ok=True)
 
     cmdline = [
         "podman", "run", "--rm",
+        # note that "-t" is missing here
         "--privileged",
-        "--security-opt", "label=type:unconfined_t",
-        "-v", f"{container_storage}:/var/lib/containers/storage",
-        "-v", f"{output_path}:/output",
         build_fake_container,
         "build",
-        # explicitly select term progress
+        # explicitly selecting term progress works even when there is no tty
+        # (i.e. we just need ansi terminal support)
         "--progress=term",
         "quay.io/centos-bootc/centos-bootc:stream9",
     ]
     res = subprocess.run(cmdline, capture_output=True, text=True, check=False)
     assert res.returncode == 0
     assert "[|] Manifest generation step" in res.stderr
+
+
+def test_progress_term_autoselect(tmp_path, build_fake_container):
+    output_path = tmp_path / "output"
+    output_path.mkdir(exist_ok=True)
+
+    cmdline = [
+        "podman", "run", "--rm",
+        # we have a terminal
+        "-t",
+        "--privileged",
+        build_fake_container,
+        "build",
+        # note that we do not select a --progress here so auto-select is used
+        "quay.io/centos-bootc/centos-bootc:stream9",
+    ]
+    res = subprocess.run(cmdline, capture_output=True, text=True, check=False)
+    assert res.returncode == 0
+    # its curious that we get the output on stdout here, podman weirdness?
+    assert "[|] Manifest generation step" in res.stdout