progress: simplify/tweak Progress interface

The Start/Stop methods can no longer error so remove the error
from the signature. The commit also tweaks the doc strings and
comments slightly.

bib/cmd/bootc-image-builder/main.go

@@ -244,9 +244,7 @@ func manifestFromCobra(cmd *cobra.Command, args []string, pbar progress.Progress
 	}
 
 	pbar.SetPulseMsgf("Manifest generation step")
-	if err := pbar.Start(); err != nil {
-		return nil, nil, fmt.Errorf("cannot start progress: %v", err)
-	}
+	pbar.Start()
 
 	if err := setup.ValidateHasContainerTags(imgref); err != nil {
 		return nil, nil, err
@@ -434,11 +432,7 @@ func cmdBuild(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return fmt.Errorf("cannto create progress bar: %w", err)
 	}
-	defer func() {
-		if err := pbar.Stop(); err != nil {
-			logrus.Warnf("progressbar stopping failed: %v", err)
-		}
-	}()
+	defer pbar.Stop()
 
 	manifest_fname := fmt.Sprintf("manifest-%s.json", strings.Join(imgTypes, "-"))
 	pbar.SetMessagef("Generating manifest %s", manifest_fname)

bib/internal/progress/progress.go

@@ -38,6 +38,10 @@ type ProgressBar interface {
 	// SetProgress sets the progress details at the given "level".
 	// Levels should start with "0" and increase as the nesting
 	// gets deeper.
+	//
+	// Note that reducing depth is currently not supported, once
+	// a sub-progress is added it cannot be removed/hidden
+	// (but if required it can be added, its a SMOP)
 	SetProgress(level int, msg string, done int, total int) error
 
 	// The high-level message that is displayed in a spinner
@@ -52,8 +56,13 @@ type ProgressBar interface {
 	// For us this usually comes from the stages and has information
 	// like "Starting module org.osbuild.selinux"
 	SetMessagef(fmt string, args ...interface{})
-	Start() error
-	Stop() error
+
+	// Start will start rendering the progress information
+	Start()
+
+	// Stop will stop rendering the progress information, the
+	// screen is not cleared, the last few lines will be visible
+	Stop()
 }
 
 // New creates a new progressbar based on the requested type
@@ -165,21 +174,19 @@ func (b *terminalProgressBar) renderLoop() {
 	}
 }
 
-func (b *terminalProgressBar) Start() error {
+func (b *terminalProgressBar) Start() {
 	// render() already running
 	if b.shutdownCh != nil {
-		return nil
+		return
 	}
 	fmt.Fprintf(b.out, "%s", CURSOR_HIDE)
 	b.shutdownCh = make(chan bool)
 	go b.renderLoop()
-
-	return nil
 }
 
-func (b *terminalProgressBar) Stop() (err error) {
+func (b *terminalProgressBar) Stop() {
 	if b.shutdownCh == nil {
-		return nil
+		return
 	}
 	// request shutdown
 	b.shutdownCh <- true
@@ -188,10 +195,12 @@ func (b *terminalProgressBar) Stop() (err error) {
 	case <-b.shutdownCh:
 	// shudown complete
 	case <-time.After(1 * time.Second):
+		// I cannot think of how this could happen, i.e. why
+		// closing would not work but lets be conservative -
+		// without a timeout we hang here forever
 		logrus.Warnf("no progress channel shutdown after 1sec")
 	}
 	b.shutdownCh = nil
-	return nil
 }
 
 type plainProgressBar struct {
@@ -213,12 +222,10 @@ func (b *plainProgressBar) SetMessagef(msg string, args ...interface{}) {
 	fmt.Fprintf(b.w, msg, args...)
 }
 
-func (b *plainProgressBar) Start() (err error) {
-	return nil
+func (b *plainProgressBar) Start() {
 }
 
-func (b *plainProgressBar) Stop() (err error) {
-	return nil
+func (b *plainProgressBar) Stop() {
 }
 
 func (b *plainProgressBar) SetProgress(subLevel int, msg string, done int, total int) error {
@@ -250,14 +257,12 @@ func (b *debugProgressBar) SetMessagef(msg string, args ...interface{}) {
 	fmt.Fprintf(b.w, "\n")
 }
 
-func (b *debugProgressBar) Start() (err error) {
+func (b *debugProgressBar) Start() {
 	fmt.Fprintf(b.w, "Start progressbar\n")
-	return nil
 }
 
-func (b *debugProgressBar) Stop() (err error) {
+func (b *debugProgressBar) Stop() {
 	fmt.Fprintf(b.w, "Stop progressbar\n")
-	return nil
 }
 
 func (b *debugProgressBar) SetProgress(subLevel int, msg string, done int, total int) error {

bib/internal/progress/progress_test.go

@@ -54,11 +54,9 @@ func TestPlainProgress(t *testing.T) {
 	assert.Equal(t, "message", buf.String())
 	buf.Reset()
 
-	err = pbar.Start()
-	assert.NoError(t, err)
+	pbar.Start()
 	assert.Equal(t, "", buf.String())
-	err = pbar.Stop()
-	assert.NoError(t, err)
+	pbar.Stop()
 	assert.Equal(t, "", buf.String())
 }
 
@@ -82,13 +80,11 @@ func TestDebugProgress(t *testing.T) {
 	assert.Equal(t, "msg: some-message\n", buf.String())
 	buf.Reset()
 
-	err = pbar.Start()
-	assert.NoError(t, err)
+	pbar.Start()
 	assert.Equal(t, "Start progressbar\n", buf.String())
 	buf.Reset()
 
-	err = pbar.Stop()
-	assert.NoError(t, err)
+	pbar.Stop()
 	assert.Equal(t, "Stop progressbar\n", buf.String())
 	buf.Reset()
 }
@@ -101,14 +97,12 @@ func TestTermProgress(t *testing.T) {
 	pbar, err := progress.NewTerminalProgressBar()
 	assert.NoError(t, err)
 
-	err = pbar.Start()
-	assert.NoError(t, err)
+	pbar.Start()
 	pbar.SetPulseMsgf("pulse-msg")
 	pbar.SetMessagef("some-message")
 	err = pbar.SetProgress(0, "set-progress-msg", 0, 5)
 	assert.NoError(t, err)
-	err = pbar.Stop()
-	assert.NoError(t, err)
+	pbar.Stop()
 
 	assert.Contains(t, buf.String(), "[1 / 6] set-progress-msg")
 	assert.Contains(t, buf.String(), "[|] pulse-msg\n")