Repository: flant/shell-operator
Pull request #514 (2023-07-15 09:01:29)

Fix queue dump output formatting
<!--
Thank you for sending a pull request! Here some tips for contributors:

1. Fill the description template below.
2. Include appropriate tests (if necessary). Make sure that all CI checks passed.
3. If the Pull Request is a work in progress, make use of GitHub's "Draft PR" feature and mark it as such.
-->

#### Overview

Avoid double marshalling on json/yaml output when calling queue dump command - `queue list`

#### What this PR does / why we need it

We have output formatter inside the router. So we have to avoid an extra marshalling when calling a queue dump

#### Special notes for your reviewer

Before this change shell-operator had output with extra marshalling and printed a json like a string:
```
'{"active":[],"summary":{"mainQueueTasks":0,"otherQueues":{"active":0,"empty":72,"tasks":0},"totalTasks":0}}'
```

now it's ok:
```
{"active":[],"summary":{"mainQueueTasks":0,"otherQueues":{"active":0,"empty":72,"tasks":0},"totalTasks":0}}
```




Patch:
diff --git a/pkg/config/config.go b/pkg/config/config.go
index 89c311d..0981c60 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -126,6 +126,23 @@ func (c *Config) Has(name string) bool {
 	return c.has(name)
 }
 
+// IsValid validates runtime config parameter
+func (c *Config) IsValid(name, value string) error {
+	// maybe we should register it together with a parameter
+	switch name {
+	case "log.level":
+		_, err := log.ParseLevel(value)
+		if err != nil {
+			return err
+		}
+
+	default:
+		return nil
+	}
+
+	return nil
+}
+
 func (c *Config) has(name string) bool {
 	_, registered := c.params[name]
 	return registered
diff --git a/pkg/debug/server.go b/pkg/debug/server.go
index f17131f..d964c3c 100644
--- a/pkg/debug/server.go
+++ b/pkg/debug/server.go
@@ -14,7 +14,7 @@ import (
 	"sigs.k8s.io/yaml"
 
 	utils "github.com/flant/shell-operator/pkg/utils/file"
-	"github.com/flant/shell-operator/pkg/utils/structured-logger"
+	structured_logger "github.com/flant/shell-operator/pkg/utils/structured-logger"
 )
 
 type Server struct {
@@ -87,7 +87,7 @@ func (s *Server) Route(pattern string, handler func(request *http.Request) (inte
 		return
 	}
 	s.Router.Get(pattern, func(writer http.ResponseWriter, request *http.Request) {
-		HandleFormattedOutput(writer, request, handler)
+		handleFormattedOutput(writer, request, handler)
 	})
 }
 
@@ -105,22 +105,25 @@ func (s *Server) RoutePOST(pattern string, handler func(request *http.Request) (
 			return
 		}
 
-		HandleFormattedOutput(writer, request, handler)
+		handleFormattedOutput(writer, request, handler)
 	})
 }
 
-func HandleFormattedOutput(writer http.ResponseWriter, request *http.Request, handler func(request *http.Request) (interface{}, error)) {
+func handleFormattedOutput(writer http.ResponseWriter, request *http.Request, handler func(request *http.Request) (interface{}, error)) {
 	out, err := handler(request)
 	if err != nil {
 		writer.WriteHeader(http.StatusInternalServerError)
 		_, _ = fmt.Fprintf(writer, "Error: %s", err)
 		return
 	}
+	if out == nil && err == nil {
+		return
+	}
 
 	format := FormatFromRequest(request)
 	structured_logger.GetLogEntry(request).Debugf("use format '%s'", format)
 
-	outBytes, err := TransformUsingFormat(out, format)
+	outBytes, err := transformUsingFormat(out, format)
 	if err != nil {
 		writer.WriteHeader(http.StatusInternalServerError)
 		_, _ = fmt.Fprintf(writer, "Error '%s' transform: %s", format, err)
@@ -130,7 +133,7 @@ func HandleFormattedOutput(writer http.ResponseWriter, request *http.Request, ha
 	_, _ = writer.Write(outBytes)
 }
 
-func TransformUsingFormat(val interface{}, format string) ([]byte, error) {
+func transformUsingFormat(val interface{}, format string) ([]byte, error) {
 	var outBytes []byte
 	var err error
 
diff --git a/pkg/shell-operator/debug_server.go b/pkg/shell-operator/debug_server.go
index d4cd847..09ff4e0 100644
--- a/pkg/shell-operator/debug_server.go
+++ b/pkg/shell-operator/debug_server.go
@@ -85,6 +85,10 @@ func RegisterDebugConfigRoutes(dbgSrv *debug.Server, runtimeConfig *config.Confi
 			return nil, fmt.Errorf("'value' parameter is required")
 		}
 
+		if err := runtimeConfig.IsValid(name, value); err != nil {
+			return nil, fmt.Errorf("'value' parameter is invalid: %w", err)
+		}
+
 		var duration time.Duration
 		var err error
 		durationStr := r.PostForm.Get("duration")
diff --git a/pkg/task/dump/dump.go b/pkg/task/dump/dump.go
index 19d9d03..a3477ca 100644
--- a/pkg/task/dump/dump.go
+++ b/pkg/task/dump/dump.go
@@ -6,8 +6,6 @@ import (
 	"sort"
 	"strings"
 
-	"sigs.k8s.io/yaml"
-
 	"github.com/flant/shell-operator/pkg/task"
 	"github.com/flant/shell-operator/pkg/task/queue"
 )
@@ -46,7 +44,8 @@ func TaskQueueMainToText(tqs *queue.TaskQueueSet) string {
 }
 
 // TaskQueues dumps all queues.
-func TaskQueues(tqs *queue.TaskQueueSet, format string, showEmpty bool) string {
+// returns empty interface because we have a formatter on the router level, which will marshal data or return it as a string
+func TaskQueues(tqs *queue.TaskQueueSet, format string, showEmpty bool) interface{} {
 	result := dumpTaskQueues{
 		Empty:  make([]dumpQueue, 0),
 		Active: make([]dumpQueue, 0),
@@ -122,7 +121,7 @@ func TaskQueues(tqs *queue.TaskQueueSet, format string, showEmpty bool) string {
 		}
 	}
 
-	return result.Format(format, showEmpty)
+	return result.format(format, showEmpty)
 }
 
 func pluralize(n int, zero, one, many string) string {
@@ -227,26 +226,16 @@ func (dtq dumpTaskQueues) SortByName() {
 	sort.Sort(asQueueNames(dtq.Empty))
 }
 
-func (dtq dumpTaskQueues) Format(format string, showEmpty bool) string {
+func (dtq dumpTaskQueues) format(format string, showEmpty bool) interface{} {
 	switch format {
 	case "text":
 		return dtq.asText(showEmpty)
 
-	case "json":
+	case "json", "yaml":
 		if !showEmpty {
 			dtq.Empty = make([]dumpQueue, 0)
 		}
-		res, _ := json.Marshal(dtq)
-
-		return string(res)
-
-	case "yaml":
-		if !showEmpty {
-			dtq.Empty = make([]dumpQueue, 0)
-		}
-		res, _ := yaml.Marshal(dtq)
-
-		return string(res)
+		return dtq
 	}
 
 	return ""


Test patch:
diff --git a/pkg/task/dump/dump_test.go b/pkg/task/dump/dump_test.go
index ed68ced..dd571e0 100644
--- a/pkg/task/dump/dump_test.go
+++ b/pkg/task/dump/dump_test.go
@@ -2,11 +2,13 @@ package dump
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"sort"
 	"testing"
 
 	. "github.com/onsi/gomega"
+	"sigs.k8s.io/yaml"
 
 	"github.com/flant/shell-operator/pkg/hook/task_metadata"
 	"github.com/flant/shell-operator/pkg/task"
@@ -73,7 +75,7 @@ func Test_Dump(t *testing.T) {
 
 	// Empty set, no queues, dump should not fail.
 	t.Run("empty set", func(t *testing.T) {
-		dump := TaskQueues(tqs, "text", true)
+		dump := testDumpQueuesWrapper(tqs, "text", true)
 		g.Expect(dump).To(ContainSubstring("No queues"))
 	})
 
@@ -82,12 +84,12 @@ func Test_Dump(t *testing.T) {
 		tqs.NewNamedQueue("main", nil)
 
 		// Single empty main should be reported only as summary.
-		dump := TaskQueues(tqs, "text", true)
+		dump := testDumpQueuesWrapper(tqs, "text", true)
 		t.Log(dump)
 		g.Expect(dump).ToNot(ContainSubstring("Empty queues"))
 		g.Expect(dump).To(ContainSubstring(": empty"))
 
-		dump = TaskQueues(tqs, "json", true)
+		dump = testDumpQueuesWrapper(tqs, "json", true)
 		t.Log(dump)
 		g.Expect(dump).To(MatchJSON(`{"active":[],"summary":{"mainQueueTasks":0,"otherQueues":{"active":0,"empty":0,"tasks":0},"totalTasks":0}}`))
 	})
@@ -96,7 +98,7 @@ func Test_Dump(t *testing.T) {
 		fillQueue(tqs.GetMain(), mainTasks)
 
 		// Main queue is not counted as active or empty queue.
-		dump := TaskQueues(tqs, "text", true)
+		dump := testDumpQueuesWrapper(tqs, "text", true)
 		g.Expect(dump).ToNot(ContainSubstring("other"))
 		g.Expect(dump).To(ContainSubstring(": %d tasks", mainTasks))
 	})
@@ -106,7 +108,7 @@ func Test_Dump(t *testing.T) {
 		tqs.NewNamedQueue("active-queue", nil)
 		fillQueue(tqs.GetByName("active-queue"), activeTasks)
 
-		dump := TaskQueues(tqs, "text", true)
+		dump := testDumpQueuesWrapper(tqs, "text", true)
 		g.Expect(dump).To(ContainSubstring("1 active"))
 		g.Expect(dump).To(ContainSubstring(": %d tasks", mainTasks))
 		g.Expect(dump).To(ContainSubstring(": %d tasks", activeTasks))
@@ -116,23 +118,42 @@ func Test_Dump(t *testing.T) {
 	t.Run("create empty queue", func(t *testing.T) {
 		tqs.NewNamedQueue("empty", nil)
 
-		dump := TaskQueues(tqs, "text", true)
+		dump := testDumpQueuesWrapper(tqs, "text", true)
 		t.Log(dump)
 		g.Expect(dump).To(ContainSubstring("1 active"))
 		g.Expect(dump).To(ContainSubstring("1 empty"))
 		g.Expect(dump).To(ContainSubstring("total %d tasks", mainTasks+activeTasks))
 
-		dump = TaskQueues(tqs, "json", true)
+		dump = testDumpQueuesWrapper(tqs, "json", true)
 		g.Expect(dump).To(MatchJSON(`{"active":[{"name":"main","tasksCount":5,"tasks":[{"index":1,"description":":::test_task_main_0004"},{"index":2,"description":":::test_task_main_0003"},{"index":3,"description":":::test_task_main_0002"},{"index":4,"description":":::test_task_main_0001"},{"index":5,"description":":::test_task_main_0000"}]},{"name":"active-queue","tasksCount":4,"tasks":[{"index":1,"description":":::test_task_active-queue_0003"},{"index":2,"description":":::test_task_active-queue_0002"},{"index":3,"description":":::test_task_active-queue_0001"},{"index":4,"description":":::test_task_active-queue_0000"}]}],"empty":[{"name":"empty","tasksCount":0}],"summary":{"mainQueueTasks":5,"otherQueues":{"active":1,"empty":1,"tasks":4},"totalTasks":9}}`))
 	})
 
 	t.Run("omit empty queue", func(t *testing.T) {
-		dump := TaskQueues(tqs, "text", false)
+		dump := testDumpQueuesWrapper(tqs, "text", false)
 		g.Expect(dump).To(ContainSubstring("1 active"))
 		g.Expect(dump).ToNot(ContainSubstring("Empty queues (1):"))
 		g.Expect(dump).To(ContainSubstring("total %d tasks", mainTasks+activeTasks))
 
-		dump = TaskQueues(tqs, "json", false)
+		dump = testDumpQueuesWrapper(tqs, "json", false)
 		g.Expect(dump).To(MatchJSON(`{"active":[{"name":"main","tasksCount":5,"tasks":[{"index":1,"description":":::test_task_main_0004"},{"index":2,"description":":::test_task_main_0003"},{"index":3,"description":":::test_task_main_0002"},{"index":4,"description":":::test_task_main_0001"},{"index":5,"description":":::test_task_main_0000"}]},{"name":"active-queue","tasksCount":4,"tasks":[{"index":1,"description":":::test_task_active-queue_0003"},{"index":2,"description":":::test_task_active-queue_0002"},{"index":3,"description":":::test_task_active-queue_0001"},{"index":4,"description":":::test_task_active-queue_0000"}]}],"summary":{"mainQueueTasks":5,"otherQueues":{"active":1,"empty":1,"tasks":4},"totalTasks":9}}`))
 	})
 }
+
+func testDumpQueuesWrapper(tqs *queue.TaskQueueSet, format string, showEmpty bool) interface{} {
+	dump := TaskQueues(tqs, format, showEmpty)
+
+	switch format {
+	case "text":
+		return dump.(string)
+
+	case "json":
+		data, _ := json.Marshal(dump)
+		return string(data)
+
+	case "yaml":
+		data, _ := yaml.Marshal(dump)
+		return string(data)
+	}
+
+	return nil
+}