Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

progress: auto-select progress based on the environment #765

Merged
merged 1 commit into from
Dec 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions bib/internal/progress/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
14 changes: 11 additions & 3 deletions bib/internal/progress/progress.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()
Expand Down
19 changes: 19 additions & 0 deletions bib/internal/progress/progress_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}
36 changes: 26 additions & 10 deletions test/test_progress.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Loading