From c1f3cfe6957304d0330492bf12f3d0f43aaa88a7 Mon Sep 17 00:00:00 2001
From: Evans Mungai <evans@replicated.com>
Date: Tue, 26 Sep 2023 12:00:30 +0100
Subject: [PATCH] Changes from PR comments

* Use tabwriter to log preflight check results
* DRY code
---
 cmd/kots/cli/install.go    |  8 +++++---
 pkg/preflight/execute.go   |  8 ++++----
 pkg/preflight/preflight.go | 13 +++++++++++--
 pkg/print/config.go        | 31 +++++++++++--------------------
 4 files changed, 31 insertions(+), 29 deletions(-)

diff --git a/cmd/kots/cli/install.go b/cmd/kots/cli/install.go
index 94061849b5..3fb564bfa1 100644
--- a/cmd/kots/cli/install.go
+++ b/cmd/kots/cli/install.go
@@ -450,12 +450,14 @@ func InstallCmd() *cobra.Command {
 				case storetypes.VersionPendingPreflight:
 					log.ActionWithSpinner("Waiting for preflight checks to complete")
 					if err := ValidatePreflightStatus(deployOptions, authSlug, apiEndpoint); err != nil {
-						log.FinishSpinnerWithError()
-						log.Errorf("Preflight checks contain warnings or errors. The application was not deployed")
 						perr := preflightError{}
 						if errors.As(err, &perr) {
+							log.FinishSpinner() // We succeeded waiting for the results. Don't finish with an error
+							log.Errorf("Preflight checks contain warnings or errors. The application was not deployed")
 							print.PreflightErrors(log, perr.Results)
-							cmd.SilenceErrors = true // Stop Cobra from printing the error, we print it ourselves
+							cmd.SilenceErrors = true // Stop Cobra from printing the error, we format the message ourselves
+						} else {
+							log.FinishSpinnerWithError()
 						}
 						return err
 					}
diff --git a/pkg/preflight/execute.go b/pkg/preflight/execute.go
index 697a068475..4b73d70648 100644
--- a/pkg/preflight/execute.go
+++ b/pkg/preflight/execute.go
@@ -66,10 +66,10 @@ func execute(appID string, sequence int64, preflightSpec *troubleshootv1beta2.Pr
 				logger.Errorf("error while running preflights: %v", err)
 			} else {
 				switch m := msg.(type) {
-					case preflight.CollectProgress:
-						logger.Infof("preflight progress: %s", m.String())
-					default:
-						logger.Infof("preflight progress: %+v", msg)
+				case preflight.CollectProgress:
+					logger.Infof("preflight progress: %s", m.String())
+				default:
+					logger.Infof("preflight progress: %+v", msg)
 				}
 			}
 
diff --git a/pkg/preflight/preflight.go b/pkg/preflight/preflight.go
index f94440dc59..edcfea6e4e 100644
--- a/pkg/preflight/preflight.go
+++ b/pkg/preflight/preflight.go
@@ -175,7 +175,7 @@ func Run(appID string, appSlug string, sequence int64, isAirgap bool, archiveDir
 					if result == nil {
 						continue
 					}
-					logger.Infof("preflight state=%s title=%q message=%q", preflightState(*result), result.Title, result.Message)
+					logger.Infof("preflight state=%s title=%q message=%q", GetPreflightCheckState(result), result.Title, result.Message)
 				}
 			} else {
 				logger.Info("preflight checks completed")
@@ -229,7 +229,12 @@ func Run(appID string, appSlug string, sequence int64, isAirgap bool, archiveDir
 	return nil
 }
 
-func preflightState(p troubleshootpreflight.UploadPreflightResult) string {
+// GetPreflightCheckState returns the state of a single preflight check result
+func GetPreflightCheckState(p *troubleshootpreflight.UploadPreflightResult) string {
+	if p == nil {
+		return "unknown"
+	}
+
 	if p.IsFail {
 		return "fail"
 	}
@@ -275,6 +280,10 @@ func maybeDeployFirstVersion(appID string, sequence int64, preflightResults *typ
 	return true, nil
 }
 
+// GetPreflightState returns a single state based on checking all
+// preflight checks results. If there are any errors, the state is fail.
+// If there are no errors and any warnings, the state is warn.
+// Otherwise, the state is pass.
 func GetPreflightState(preflightResults *types.PreflightResults) string {
 	if len(preflightResults.Errors) > 0 {
 		return "fail"
diff --git a/pkg/print/config.go b/pkg/print/config.go
index 619c8ef0d6..3507dcc75a 100644
--- a/pkg/print/config.go
+++ b/pkg/print/config.go
@@ -6,7 +6,8 @@ import (
 
 	configtypes "github.com/replicatedhq/kots/pkg/kotsadmconfig/types"
 	"github.com/replicatedhq/kots/pkg/logger"
-	"github.com/replicatedhq/troubleshoot/pkg/preflight"
+	"github.com/replicatedhq/kots/pkg/preflight"
+	tsPreflight "github.com/replicatedhq/troubleshoot/pkg/preflight"
 )
 
 func ConfigValidationErrors(log *logger.CLILogger, groupValidationErrors []configtypes.ConfigGroupValidationError) {
@@ -28,25 +29,15 @@ func ConfigValidationErrors(log *logger.CLILogger, groupValidationErrors []confi
 	log.Errorf(sb.String())
 }
 
-func PreflightErrors(log *logger.CLILogger, results []*preflight.UploadPreflightResult) {
-	var s strings.Builder
-	s.WriteString("\nPreflight check results (state - title - message)")
-	for _, result := range results {
-		s.WriteString(fmt.Sprintf("\n%s - %q - %q", preflightState(result), result.Title, result.Message))
-	}
-	log.Info(s.String())
-}
+func PreflightErrors(log *logger.CLILogger, results []*tsPreflight.UploadPreflightResult) {
+	w := NewTabWriter()
+	defer w.Flush()
 
-func preflightState(r *preflight.UploadPreflightResult) string {
-	if r.IsFail {
-		return "FAIL"
-	}
-	if r.IsWarn {
-		return "WARN"
-	}
-	if r.IsPass {
-		return "PASS"
+	fmt.Fprintf(w, "\n")
+	fmtColumns := "%s\t%s\t%s\n"
+	fmt.Fprintf(w, fmtColumns, "STATE", "TITLE", "MESSAGE")
+	for _, result := range results {
+		fmt.Fprintf(w, fmtColumns, strings.ToUpper(preflight.GetPreflightCheckState(result)), result.Title, result.Message)
 	}
-	// We should never get here
-	return "UNKNOWN"
+	fmt.Fprintf(w, "\n")
 }