Make restore logs testable by returning errors (#10234)

pkg/cmd/cli/restore/logs.go was the last command in the CLI still calling
cmd.Exit, which calls os.Exit directly. Two of its own tests were skipped
because of it, and said so:

    t.Skip("Cannot test restore not complete case due to cmd.Exit() call")

This gives restore logs the LogsOptions shape that backup logs already
uses: Complete, BindFlags and Run returning an error, with the cobra
command passing that to cmd.CheckError. Both skipped tests now run and
assert on the returned errors.

Exit status is unchanged; cmd.CheckError also exits 1. The two refusal
messages now carry the standard "An error occurred:" prefix and match
the wording backup logs uses.

Signed-off-by: saral <ilovegojo2580@gmail.com>

changelogs/unreleased/10234-Ralthos

@@ -0,0 +1 @@
+Make velero restore logs return errors instead of calling os.Exit directly, matching velero backup logs, and enable the two previously skipped tests

pkg/cmd/cli/restore/logs.go

@@ -23,8 +23,9 @@ import (
 	"time"
 
 	"github.com/spf13/cobra"
+	"github.com/spf13/pflag"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
+	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	"github.com/vmware-tanzu/velero/pkg/client"
@@ -34,59 +35,94 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/cmd/util/downloadrequest"
 )
 
-func NewLogsCommand(f client.Factory) *cobra.Command {
+// LogsOptions holds the state for the restore logs command, mirroring
+// pkg/cmd/cli/backup.LogsOptions so both commands are shaped the same way.
+type LogsOptions struct {
+	Timeout               time.Duration
+	InsecureSkipTLSVerify bool
+	CaCertFile            string
+	Client                kbclient.Client
+	RestoreName           string
+}
+
+func NewLogsOptions() LogsOptions {
 	config, err := client.LoadConfig()
 	if err != nil {
 		fmt.Fprintf(os.Stderr, "WARNING: Error reading config file: %v\n", err)
 	}
 
-	timeout := time.Minute
-	insecureSkipTLSVerify := false
-	caCertFile := config.CACertFile()
+	return LogsOptions{
+		Timeout:               time.Minute,
+		InsecureSkipTLSVerify: false,
+		CaCertFile:            config.CACertFile(),
+	}
+}
+
+func (l *LogsOptions) BindFlags(flags *pflag.FlagSet) {
+	flags.DurationVar(&l.Timeout, "timeout", l.Timeout, "How long to wait to receive logs.")
+	flags.BoolVar(&l.InsecureSkipTLSVerify, "insecure-skip-tls-verify", l.InsecureSkipTLSVerify, "If true, the object store's TLS certificate will not be checked for validity. This is insecure and susceptible to man-in-the-middle attacks. Not recommended for production.")
+	flags.StringVar(&l.CaCertFile, "cacert", l.CaCertFile, "Path to a certificate bundle to use when verifying TLS connections. If not specified, the CA certificate from the BackupStorageLocation will be used if available.")
+}
+
+func (l *LogsOptions) Run(c *cobra.Command, f client.Factory) error {
+	restore := new(velerov1api.Restore)
+	err := l.Client.Get(context.Background(), kbclient.ObjectKey{Namespace: f.Namespace(), Name: l.RestoreName}, restore)
+	if apierrors.IsNotFound(err) {
+		return fmt.Errorf("restore %q does not exist", l.RestoreName)
+	} else if err != nil {
+		return fmt.Errorf("error checking for restore %q: %v", l.RestoreName, err)
+	}
+
+	switch restore.Status.Phase {
+	case velerov1api.RestorePhaseCompleted, velerov1api.RestorePhaseFailed, velerov1api.RestorePhasePartiallyFailed, velerov1api.RestorePhaseWaitingForPluginOperations, velerov1api.RestorePhaseWaitingForPluginOperationsPartiallyFailed:
+		// terminal and waiting for plugin operations phases, do nothing.
+	default:
+		return fmt.Errorf("logs for restore %q are not available until it's finished processing, please wait "+
+			"until the restore has a phase of Completed or Failed and try again", l.RestoreName)
+	}
+
+	// Get BSL cacert if available
+	bslCACert, err := cacert.GetCACertFromRestore(context.Background(), l.Client, f.Namespace(), restore)
+	if err != nil {
+		// Log the error but don't fail - we can still try to download without the BSL cacert
+		fmt.Fprintf(os.Stderr, "WARNING: Error getting cacert from BSL: %v\n", err)
+		bslCACert = ""
+	}
+
+	return downloadrequest.StreamWithBSLCACert(context.Background(), l.Client, f.Namespace(), l.RestoreName, velerov1api.DownloadTargetKindRestoreLog, os.Stdout, l.Timeout, l.InsecureSkipTLSVerify, l.CaCertFile, bslCACert)
+}
+
+func (l *LogsOptions) Complete(args []string, f client.Factory) error {
+	if len(args) > 0 {
+		l.RestoreName = args[0]
+	}
+
+	kbClient, err := f.KubebuilderClient()
+	if err != nil {
+		return err
+	}
+	l.Client = kbClient
+	return nil
+}
+
+func NewLogsCommand(f client.Factory) *cobra.Command {
+	l := NewLogsOptions()
 
 	c := &cobra.Command{
 		Use:   "logs RESTORE",
 		Short: "Get restore logs",
 		Args:  cobra.ExactArgs(1),
 		Run: func(c *cobra.Command, args []string) {
-			restoreName := args[0]
-
-			kbClient, err := f.KubebuilderClient()
+			err := l.Complete(args, f)
 			cmd.CheckError(err)
 
-			restore := new(velerov1api.Restore)
-			err = kbClient.Get(context.Background(), ctrlclient.ObjectKey{Namespace: f.Namespace(), Name: restoreName}, restore)
-			if apierrors.IsNotFound(err) {
-				cmd.Exit("Restore %q does not exist.", restoreName)
-			} else if err != nil {
-				cmd.Exit("Error checking for restore %q: %v", restoreName, err)
-			}
-
-			switch restore.Status.Phase {
-			case velerov1api.RestorePhaseCompleted, velerov1api.RestorePhaseFailed, velerov1api.RestorePhasePartiallyFailed, velerov1api.RestorePhaseWaitingForPluginOperations, velerov1api.RestorePhaseWaitingForPluginOperationsPartiallyFailed:
-				// terminal and waiting for plugin operations phases, don't exit.
-			default:
-				cmd.Exit("Logs for restore %q are not available until it's finished processing. Please wait "+
-					"until the restore has a phase of Completed or Failed and try again.", restoreName)
-			}
-
-			// Get BSL cacert if available
-			bslCACert, err := cacert.GetCACertFromRestore(context.Background(), kbClient, f.Namespace(), restore)
-			if err != nil {
-				// Log the error but don't fail - we can still try to download without the BSL cacert
-				fmt.Fprintf(os.Stderr, "WARNING: Error getting cacert from BSL: %v\n", err)
-				bslCACert = ""
-			}
-
-			err = downloadrequest.StreamWithBSLCACert(context.Background(), kbClient, f.Namespace(), restoreName, velerov1api.DownloadTargetKindRestoreLog, os.Stdout, timeout, insecureSkipTLSVerify, caCertFile, bslCACert)
+			err = l.Run(c, f)
 			cmd.CheckError(err)
 		},
 	}
 
 	c.ValidArgsFunction = cli.CompleteRestoreNames(f)
-	c.Flags().DurationVar(&timeout, "timeout", timeout, "How long to wait to receive logs.")
-	c.Flags().BoolVar(&insecureSkipTLSVerify, "insecure-skip-tls-verify", insecureSkipTLSVerify, "If true, the object store's TLS certificate will not be checked for validity. This is insecure and susceptible to man-in-the-middle attacks. Not recommended for production.")
-	c.Flags().StringVar(&caCertFile, "cacert", caCertFile, "Path to a certificate bundle to use when verifying TLS connections. If not specified, the CA certificate from the BackupStorageLocation will be used if available.")
+	l.BindFlags(c.Flags())
 
 	return c
 }

pkg/cmd/cli/restore/logs_test.go

@@ -17,10 +17,12 @@ limitations under the License.
 package restore
 
 import (
+	"fmt"
 	"os"
 	"testing"
 	"time"
 
+	flag "github.com/spf13/pflag"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	kbclient "sigs.k8s.io/controller-runtime/pkg/client"
@@ -77,13 +79,20 @@ func TestNewLogsCommand(t *testing.T) {
 		c := NewLogsCommand(f)
 		assert.Equal(t, "Get restore logs", c.Short)
 
-		// The restore command exits with an error message when restore is not complete
-		// We can't easily test this since it calls cmd.Exit, which exits the process
-		// So we'll skip this test case
-		t.Skip("Cannot test restore not complete case due to cmd.Exit() call")
+		l := NewLogsOptions()
+		flags := new(flag.FlagSet)
+		l.BindFlags(flags)
+		err = l.Complete([]string{restoreName}, f)
+		require.NoError(t, err)
+
+		err = l.Run(c, f)
+		require.Error(t, err)
+		require.ErrorContains(t, err, fmt.Sprintf("logs for restore %q are not available until it's finished processing", restoreName))
 	})
 
 	t.Run("Restore not exist test", func(t *testing.T) {
+		restoreName := "not-exist"
+
 		// create a factory
 		f := &factorymocks.Factory{}
 
@@ -95,10 +104,15 @@ func TestNewLogsCommand(t *testing.T) {
 		c := NewLogsCommand(f)
 		assert.Equal(t, "Get restore logs", c.Short)
 
-		// The restore command exits with an error message when restore doesn't exist
-		// We can't easily test this since it calls cmd.Exit, which exits the process
-		// So we'll skip this test case
-		t.Skip("Cannot test restore not exist case due to cmd.Exit() call")
+		l := NewLogsOptions()
+		flags := new(flag.FlagSet)
+		l.BindFlags(flags)
+		err := l.Complete([]string{restoreName}, f)
+		require.NoError(t, err)
+
+		err = l.Run(c, f)
+		require.Error(t, err)
+		require.Equal(t, fmt.Sprintf("restore %q does not exist", restoreName), err.Error())
 	})
 
 	t.Run("Restore with BSL cacert test", func(t *testing.T) {