fix: shouldn't set error in optional phases

Signed-off-by: Jack Yu <[email protected]>

Author: Yu-Jack

pkg/manager/manager.go

@@ -160,41 +160,49 @@ func (m *SupportBundleManager) runAllPhases(requiredPhases []RunPhase, optionalP
 	maxProgressCount := len(requiredPhases) + len(optionalPhases) + len(postPhases)
 
 	for _, phase := range requiredPhases {
-		if err := m.runPhase(phase, &progressCount, maxProgressCount); err != nil {
+		if err := m.runPhase(phase, &progressCount, maxProgressCount, true); err != nil {
 			logrus.Errorf("Failed to run requiredPhases %s: %s", phase.Name, err.Error())
 			return
 		}
 	}
 
 	for _, phase := range optionalPhases {
-		if err := m.runPhase(phase, &progressCount, maxProgressCount); err != nil {
-			logrus.Errorf("Failed to run optionalPhases %s: %s", phase.Name, err.Error())
+		if err := m.runPhase(phase, &progressCount, maxProgressCount, false); err != nil {
+			logrus.Errorf("Failed to run optionalPhases %s: %s, but error is skiped", phase.Name, err.Error())
 			// Since it's optional, don't return error.
 			continue
 		}
 	}
 
 	for _, phase := range postPhases {
-		if err := m.runPhase(phase, &progressCount, maxProgressCount); err != nil {
+		if err := m.runPhase(phase, &progressCount, maxProgressCount, true); err != nil {
 			logrus.Errorf("Failed to run postPhases %s: %s", phase.Name, err.Error())
 			return
 		}
 	}
 }
 
-func (m *SupportBundleManager) runPhase(phase RunPhase, progressCount *int, maxProgressCount int) error {
+func (m *SupportBundleManager) runPhase(phase RunPhase, progressCount *int, maxProgressCount int, setError bool) error {
 	logrus.Infof("Running phase %s", phase.Name)
 	m.status.SetPhase(phase.Name)
-	if err := phase.Run(); err != nil {
-		m.status.SetError(err.Error())
-		logrus.Errorf("Failed to run phase %s: %s", phase.Name, err.Error())
-		return err
+
+	err := phase.Run()
+	if err != nil {
+		if setError {
+			m.status.SetError(err.Error())
+			logrus.Errorf("Failed to run phase %s: %s", phase.Name, err.Error())
+			return err
+		}
 	}
+
 	*progressCount++
 	progress := 100 * (*progressCount) / maxProgressCount
 	m.status.SetProgress(progress)
-	logrus.Infof("Succeed to run phase %s. Progress (%d).", phase.Name, progress)
-	return nil
+
+	if err == nil {
+		logrus.Infof("Succeed to run phase %s. Progress (%d).", phase.Name, progress)
+	}
+	return err
 }
 
 func (m *SupportBundleManager) phaseInit() error {

pkg/manager/manager_test.go

@@ -1,11 +1,15 @@
 package manager
 
 import (
+	"errors"
 	"fmt"
 	"reflect"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
 	corev1 "k8s.io/api/core/v1"
+
+	"github.com/rancher/support-bundle-kit/pkg/types"
 )
 
 func TestParseToleration(t *testing.T) {
@@ -71,3 +75,106 @@ func TestParseToleration(t *testing.T) {
 		}
 	}
 }
+
+func TestRunAllPhases(t *testing.T) {
+	tests := []struct {
+		name             string
+		requiredPhases   []RunPhase
+		optionalPhases   []RunPhase
+		postPhases       []RunPhase
+		expectedError    bool
+		expectedProgress int
+	}{
+		{
+			name: "All pass",
+			requiredPhases: []RunPhase{
+				{Name: types.ManagerPhaseInit, Run: func() error { return nil }},
+				{Name: types.ManagerPhaseClusterBundle, Run: func() error { return nil }},
+			},
+			optionalPhases: []RunPhase{
+				{Name: types.ManagerPhasePrometheusBundle, Run: func() error { return nil }},
+			},
+			postPhases: []RunPhase{
+				{Name: types.ManagerPhasePackaging, Run: func() error { return nil }},
+				{Name: types.ManagerPhaseDone, Run: func() error { return nil }},
+			},
+			expectedError:    false,
+			expectedProgress: 100,
+		},
+		{
+			name: "First Required phase error",
+			requiredPhases: []RunPhase{
+				{Name: types.ManagerPhaseInit, Run: func() error { return errors.New("required phase error") }},
+				{Name: types.ManagerPhaseClusterBundle, Run: func() error { return nil }},
+			},
+			optionalPhases: []RunPhase{
+				{Name: types.ManagerPhasePrometheusBundle, Run: func() error { return nil }},
+			},
+			postPhases: []RunPhase{
+				{Name: types.ManagerPhasePackaging, Run: func() error { return nil }},
+				{Name: types.ManagerPhaseDone, Run: func() error { return nil }},
+			},
+			expectedError:    true,
+			expectedProgress: 0,
+		},
+		{
+			name: "Second Required phase error",
+			requiredPhases: []RunPhase{
+				{Name: types.ManagerPhaseInit, Run: func() error { return nil }},
+				{Name: types.ManagerPhaseClusterBundle, Run: func() error { return errors.New("required phase error") }},
+			},
+			optionalPhases: []RunPhase{
+				{Name: types.ManagerPhasePrometheusBundle, Run: func() error { return nil }},
+			},
+			postPhases: []RunPhase{
+				{Name: types.ManagerPhasePackaging, Run: func() error { return nil }},
+				{Name: types.ManagerPhaseDone, Run: func() error { return nil }},
+			},
+			expectedError:    true,
+			expectedProgress: 20,
+		},
+		{
+			name: "Optional phase error",
+			requiredPhases: []RunPhase{
+				{Name: types.ManagerPhaseInit, Run: func() error { return nil }},
+				{Name: types.ManagerPhaseClusterBundle, Run: func() error { return nil }},
+			},
+			optionalPhases: []RunPhase{
+				{Name: types.ManagerPhasePrometheusBundle, Run: func() error { return errors.New("optional phase error") }},
+			},
+			postPhases: []RunPhase{
+				{Name: types.ManagerPhasePackaging, Run: func() error { return nil }},
+				{Name: types.ManagerPhaseDone, Run: func() error { return nil }},
+			},
+			expectedError:    false,
+			expectedProgress: 100,
+		},
+		{
+			name: "Final Post phase error",
+			requiredPhases: []RunPhase{
+				{Name: types.ManagerPhaseInit, Run: func() error { return nil }},
+				{Name: types.ManagerPhaseClusterBundle, Run: func() error { return nil }},
+			},
+			optionalPhases: []RunPhase{
+				{Name: types.ManagerPhasePrometheusBundle, Run: func() error { return nil }},
+			},
+			postPhases: []RunPhase{
+				{Name: types.ManagerPhasePackaging, Run: func() error { return nil }},
+				{Name: types.ManagerPhaseDone, Run: func() error { return errors.New("post phase error") }},
+			},
+			expectedError:    true,
+			expectedProgress: 80,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			m := &SupportBundleManager{
+				status: ManagerStatus{},
+			}
+			m.runAllPhases(tt.requiredPhases, tt.optionalPhases, tt.postPhases)
+			assert.Equal(t, tt.expectedError, m.status.Error, "expected error %v, got %v")
+			assert.Equal(t, tt.expectedProgress, m.status.Progress, "expected progress %v, got %v")
+		})
+	}
+}