[MM-65830] Fix mmctl system status exit code for health check failures (#33970) (#34069)

Automatic Merge

server/cmd/mmctl/commands/root.go

@@ -65,7 +65,14 @@ func Run(args []string) error {
 		}
 	}()
 
-	return RootCmd.Execute()
+	err := RootCmd.Execute()
+	// Flush the printer first before printing any error
+	_ = printer.Flush()
+	if err != nil {
+		_, _ = fmt.Fprintf(os.Stderr, "Error: %s\n", err.Error())
+	}
+
+	return err
 }
 
 func printPanic(x any) {
@@ -114,8 +121,6 @@ var RootCmd = &cobra.Command{
 			printer.PrintError(fmt.Sprintf("Per page value is greater than the maximum allowed. Mattermost might only return %d items.", MaxPageSize))
 		}
 	},
-	PersistentPostRun: func(cmd *cobra.Command, args []string) {
-		_ = printer.Flush()
-	},
-	SilenceUsage: true,
+	SilenceUsage:  true,
+	SilenceErrors: true,
 }

server/cmd/mmctl/commands/system.go

@@ -169,6 +169,17 @@ Ios Minimum Version: {{.IosMinVersion}}
 Database Status: {{.database_status}}
 Filestore Status: {{.filestore_status}}`, status)
 
+	// Check health status and return non-zero exit code if any component is unhealthy
+	if status["status"] != model.StatusOk {
+		return fmt.Errorf("server status is unhealthy: %s", status["status"])
+	}
+	if dbStatus := status["database_status"]; dbStatus != "" && dbStatus != model.StatusOk {
+		return fmt.Errorf("database status is unhealthy: %s", dbStatus)
+	}
+	if filestoreStatus := status["filestore_status"]; filestoreStatus != "" && filestoreStatus != model.StatusOk {
+		return fmt.Errorf("filestore status is unhealthy: %s", filestoreStatus)
+	}
+
 	return nil
 }
 

server/cmd/mmctl/commands/system_test.go

@@ -180,7 +180,31 @@ func (s *MmctlUnitTestSuite) TestServerVersionCmd() {
 }
 
 func (s *MmctlUnitTestSuite) TestServerStatusCmd() {
-	s.Run("Print server status", func() {
+	s.Run("Print server status - all healthy", func() {
+		printer.Clean()
+
+		expectedStatus := map[string]string{
+			"status":           model.StatusOk,
+			"database_status":  model.StatusOk,
+			"filestore_status": model.StatusOk,
+		}
+		s.client.
+			EXPECT().
+			GetPingWithOptions(context.TODO(), model.SystemPingOptions{
+				FullStatus:    true,
+				RESTSemantics: true,
+			}).
+			Return(expectedStatus, &model.Response{}, nil).
+			Times(1)
+
+		err := systemStatusCmdF(s.client, &cobra.Command{}, []string{})
+		s.Require().Nil(err)
+		s.Require().Len(printer.GetErrorLines(), 0)
+		s.Require().Len(printer.GetLines(), 1)
+		s.Require().Equal(printer.GetLines()[0], expectedStatus)
+	})
+
+	s.Run("Status fields missing - should succeed", func() {
 		printer.Clean()
 
 		expectedStatus := map[string]string{"status": "OK"}
@@ -217,6 +241,104 @@ func (s *MmctlUnitTestSuite) TestServerStatusCmd() {
 		s.Require().Len(printer.GetErrorLines(), 0)
 		s.Require().Len(printer.GetLines(), 0)
 	})
+
+	s.Run("Empty string database status is ignored", func() {
+		printer.Clean()
+
+		emptyDbStatus := map[string]string{
+			"status":           model.StatusOk,
+			"database_status":  "",
+			"filestore_status": model.StatusOk,
+		}
+		s.client.
+			EXPECT().
+			GetPingWithOptions(context.TODO(), model.SystemPingOptions{
+				FullStatus:    true,
+				RESTSemantics: true,
+			}).
+			Return(emptyDbStatus, &model.Response{}, nil).
+			Times(1)
+
+		err := systemStatusCmdF(s.client, &cobra.Command{}, []string{})
+		s.Require().Nil(err)
+		s.Require().Len(printer.GetErrorLines(), 0)
+		s.Require().Len(printer.GetLines(), 1)
+	})
+
+	s.Run("Unhealthy server status should return true", func() {
+		printer.Clean()
+
+		unhealthyStatus := map[string]string{
+			"status":           model.StatusUnhealthy,
+			"database_status":  model.StatusOk,
+			"filestore_status": model.StatusOk,
+		}
+		s.client.
+			EXPECT().
+			GetPingWithOptions(context.TODO(), model.SystemPingOptions{
+				FullStatus:    true,
+				RESTSemantics: true,
+			}).
+			Return(unhealthyStatus, &model.Response{}, nil).
+			Times(1)
+
+		err := systemStatusCmdF(s.client, &cobra.Command{}, []string{})
+		s.Require().Error(err)
+		s.Require().Contains(err.Error(), "server status is unhealthy")
+		s.Require().Len(printer.GetErrorLines(), 0)
+		s.Require().Len(printer.GetLines(), 1)
+		s.Require().Equal(printer.GetLines()[0], unhealthyStatus)
+	})
+
+	s.Run("Unhealthy database status should return true", func() {
+		printer.Clean()
+
+		unhealthyStatus := map[string]string{
+			"status":           model.StatusOk,
+			"database_status":  model.StatusUnhealthy,
+			"filestore_status": model.StatusOk,
+		}
+		s.client.
+			EXPECT().
+			GetPingWithOptions(context.TODO(), model.SystemPingOptions{
+				FullStatus:    true,
+				RESTSemantics: true,
+			}).
+			Return(unhealthyStatus, &model.Response{}, nil).
+			Times(1)
+
+		err := systemStatusCmdF(s.client, &cobra.Command{}, []string{})
+		s.Require().Error(err)
+		s.Require().Contains(err.Error(), "database status is unhealthy")
+		s.Require().Len(printer.GetErrorLines(), 0)
+		s.Require().Len(printer.GetLines(), 1)
+		s.Require().Equal(printer.GetLines()[0], unhealthyStatus)
+	})
+
+	s.Run("Unhealthy filestore status should return true", func() {
+		printer.Clean()
+
+		unhealthyStatus := map[string]string{
+			"status":           model.StatusOk,
+			"database_status":  model.StatusOk,
+			"filestore_status": model.StatusUnhealthy,
+		}
+		s.client.
+			EXPECT().
+			GetPingWithOptions(context.TODO(), model.SystemPingOptions{
+				FullStatus:    true,
+				RESTSemantics: true,
+			}).
+			Return(unhealthyStatus, &model.Response{}, nil).
+			Times(1)
+
+		err := systemStatusCmdF(s.client, &cobra.Command{}, []string{})
+		s.Require().Error(err)
+		s.Require().Contains(err.Error(), "filestore status is unhealthy")
+		s.Require().Len(printer.GetErrorLines(), 0)
+		s.Require().Len(printer.GetLines(), 1)
+		s.Require().Equal(printer.GetLines()[0], unhealthyStatus)
+	})
 }
 
 func (s *MmctlUnitTestSuite) TestSupportPacketCmdF() {