Repository: doitintl/kube-no-trouble
Pull request #134 (2021-04-20 14:06:03)

ref: Printer Updates
Was very WIP

Signed-off-by: david-doit-intl <david@doit-intl.com>

stepanstipl: First, can we pls. rebase/merge - currently PR is showing diffs for changes that have landed in master _(I've tried to change base back & forth ^^, but that didn't seem to help)_.

Now I would like to keep the conversation about the refactoring and _jsonl_ separate, otherwise, it will get mixed together. So my proposal is move `jsonl` related changes to another PR (they would need to be in separate commits anyway), and let's discuss the refactor here.

Re. refactor: I like the general direction of moving the logic away from `main` 👍 .

What I'm not sure about is introducing the new type `PrinterType`:
```go
type PrinterType struct {
	Printer printer.Printer
	name    string
}
```

It doesn't seem to bring much value, and I think we can get away with using the `Printer` type, that already exists, directly in the config to hold the pointer to the `Printer` instance. And not introduce this additional abstraction layer. But maybe I'm missing smth.?

`+` thanks for the added tests! 🎉 
david-doit-intl: @stepanstipl I am pretty sure this pr is too date. Not sure what exactly you are seeing. I am happy to move the jsonl out no worries!
david-doit-intl: @stepanstipl printer is an interface not a concrete type so we need something to hold it which is concrete, additionally the name stores what printer you selected, I also plan to add args to the struct at a later date, so the struct seems the most reasonable
david-doit-intl: Additionally I want to make the printer type less tightly coupled to the printer module
stepanstipl: > @stepanstipl I am pretty sure this pr is too date. Not sure what exactly you are seeing.

There are some log level changes in the diff - https://github.com/doitintl/kube-no-trouble/pull/134/files#diff-e1976a6dedca1ef99072ad60854c3e58bd53e68b07dc7a04cb0102e08a115623

> I am happy to move the jsonl out no worries!

👍 , thanks.

> @stepanstipl printer is an interface not a concrete type so we need something to hold it which is concrete, additionally the name stores what printer you selected, I also plan to add args to the struct at a later date, so the struct seems the most reasonable

So in the current implementation, the config holds `PrinterType`:
```go
		Output: config.PrinterType{
			Printer: &printer.TextPrinter{},
		},
```
and this, in turn, holds pointer to an instance of some type satisfying the `Printer` interface:
```go
type PrinterType struct {
	Printer printer.Printer
	name    string
}
```
So it's `Config.Output -----> PrinterType -----> *Printer`.

We can do just `Config.Output ----->*Printer`.


The name can become property/method of the Printer interface
david-doit-intl: @stepanstipl no, thats the who point. Printer is a interface not a concrete type.
david-doit-intl: though I am happy to make name part of the interface
david-doit-intl: @stepanstipl have a look now
david-doit-intl: @stepanstipl much happier now :)
david-doit-intl: Ok all done.
stepanstipl: Yep, looks good 👍 , can you please do the final housekeeping before the merge & I will merge it:
> Some housekeeping details (can be done later, before final merge):
> 
> probably should be `ref: `commit, as refactoring
> split the zerolog related changes to a separate `ref: ` commit

`+` rebase on current master

david-doit-intl: > Yep, looks good  , can you please do the final housekeeping before the merge & I will merge it:
> 
> > Some housekeeping details (can be done later, before final merge):
> > probably should be `ref: `commit, as refactoring
> > split the zerolog related changes to a separate `ref: ` commit
> 
> `+` rebase on current master

Yes forgot my bad

Patch:
diff --git a/cmd/kubent/main.go b/cmd/kubent/main.go
index 46e9e6f..c710191 100644
--- a/cmd/kubent/main.go
+++ b/cmd/kubent/main.go
@@ -119,18 +119,12 @@ func main() {
 		log.Fatal().Err(err).Str("name", "Rego").Msg("Failed to evaluate input")
 	}
 
-	var prnt printer.Printer
-	switch config.Output {
-	case "json":
-		prnt, err = printer.NewJSONPrinter(&printer.JSONOpts{})
-	default:
-		prnt, err = printer.NewTextPrinter(&printer.TextOpts{})
-	}
+	printer, err := printer.NewPrinter(config.Output)
 	if err != nil {
-		log.Fatal().Err(err).Str("name", config.Output).Msg("Failed to initialize output printer")
+		log.Fatal().Err(err).Msg("Failed to create printer")
 	}
 
-	err = prnt.Print(results)
+	err = printer.Print(results)
 	if err != nil {
 		log.Fatal().Err(err).Msg("Failed to print results")
 	}
diff --git a/pkg/config/config.go b/pkg/config/config.go
index f19d468..f145cee 100644
--- a/pkg/config/config.go
+++ b/pkg/config/config.go
@@ -7,30 +7,12 @@ import (
 	"strings"
 	"unicode"
 
+	"github.com/doitintl/kube-no-trouble/pkg/printer"
 	"github.com/rs/zerolog"
 	flag "github.com/spf13/pflag"
 	"k8s.io/client-go/util/homedir"
 )
 
-type ZeroLogLevel int8
-
-func (l ZeroLogLevel) String() string {
-	return zerolog.Level(l).String()
-}
-
-func (l *ZeroLogLevel) Set(s string) error {
-	level, err := zerolog.ParseLevel(s)
-	if err != nil {
-		return err
-	}
-	*l = ZeroLogLevel(level)
-	return nil
-}
-
-func (l ZeroLogLevel) Type() string {
-	return "string"
-}
-
 type Config struct {
 	AdditionalKinds []string
 	Cluster         bool
@@ -44,7 +26,9 @@ type Config struct {
 }
 
 func NewFromFlags() (*Config, error) {
-	config := Config{LogLevel: ZeroLogLevel(zerolog.InfoLevel)}
+	config := Config{
+		LogLevel: ZeroLogLevel(zerolog.InfoLevel),
+	}
 
 	home := homedir.HomeDir()
 	flag.StringSliceVarP(&config.AdditionalKinds, "additional-kind", "a", []string{}, "additional kinds of resources to report in Kind.version.group.com format")
@@ -59,6 +43,10 @@ func NewFromFlags() (*Config, error) {
 
 	flag.Parse()
 
+	if _, err := printer.ParsePrinter(config.Output); err != nil {
+		return nil, fmt.Errorf("failed to validate argument output: %w", err)
+	}
+
 	if err := validateAdditionalResources(config.AdditionalKinds); err != nil {
 		return nil, fmt.Errorf("failed to validate arguments: %w", err)
 	}
diff --git a/pkg/config/zerologlevel.go b/pkg/config/zerologlevel.go
new file mode 100644
index 0000000..ae06d37
--- /dev/null
+++ b/pkg/config/zerologlevel.go
@@ -0,0 +1,24 @@
+package config
+
+import (
+	"github.com/rs/zerolog"
+)
+
+type ZeroLogLevel int8
+
+func (l ZeroLogLevel) String() string {
+	return zerolog.Level(l).String()
+}
+
+func (l *ZeroLogLevel) Set(s string) error {
+	level, err := zerolog.ParseLevel(s)
+	if err != nil {
+		return err
+	}
+	*l = ZeroLogLevel(level)
+	return nil
+}
+
+func (l ZeroLogLevel) Type() string {
+	return "string"
+}
diff --git a/pkg/printer/json.go b/pkg/printer/json.go
index 6e7070f..a17881f 100644
--- a/pkg/printer/json.go
+++ b/pkg/printer/json.go
@@ -8,29 +8,31 @@ import (
 	"github.com/doitintl/kube-no-trouble/pkg/judge"
 )
 
-type JSONPrinter struct {
+type jsonPrinter struct {
 }
 
-type JSONOpts struct {
+func newJSONPrinter() Printer {
+	return &jsonPrinter{}
 }
 
-func NewJSONPrinter(opts *JSONOpts) (*JSONPrinter, error) {
-	printer := &JSONPrinter{}
-
-	return printer, nil
-}
-
-func (c *JSONPrinter) Print(results []judge.Result) error {
-
+func (c *jsonPrinter) populateOutput(results []judge.Result) (string, error) {
 	buffer := new(bytes.Buffer)
 	encoder := json.NewEncoder(buffer)
 	encoder.SetIndent("", "\t")
 
 	err := encoder.Encode(results)
+	if err != nil {
+		return "", err
+	}
+	return buffer.String(), nil
+}
+
+func (c *jsonPrinter) Print(results []judge.Result) error {
+	output, err := c.populateOutput(results)
 	if err != nil {
 		return err
 	}
-	fmt.Printf("%s", buffer.String())
+	fmt.Printf("%s", output)
 
 	return nil
 }
diff --git a/pkg/printer/printer.go b/pkg/printer/printer.go
index 00bce0b..3837244 100644
--- a/pkg/printer/printer.go
+++ b/pkg/printer/printer.go
@@ -1,9 +1,32 @@
 package printer
 
 import (
+	"fmt"
+
 	"github.com/doitintl/kube-no-trouble/pkg/judge"
 )
 
+var printers = map[string]func() Printer{
+	"json": newJSONPrinter,
+	"text": newTextPrinter,
+}
+
 type Printer interface {
 	Print([]judge.Result) error
 }
+
+func NewPrinter(choice string) (Printer, error) {
+	printer, err := ParsePrinter(choice)
+	if err != nil {
+		return nil, err
+	}
+	return printer(), nil
+}
+
+func ParsePrinter(choice string) (func() Printer, error) {
+	printer, exists := printers[choice]
+	if !exists {
+		return nil, fmt.Errorf("unknown printer type %s", choice)
+	}
+	return printer, nil
+}
diff --git a/pkg/printer/text.go b/pkg/printer/text.go
index dba301e..65b7486 100644
--- a/pkg/printer/text.go
+++ b/pkg/printer/text.go
@@ -10,19 +10,14 @@ import (
 	"github.com/doitintl/kube-no-trouble/pkg/judge"
 )
 
-type TextPrinter struct {
+type textPrinter struct {
 }
 
-type TextOpts struct {
+func newTextPrinter() Printer {
+	return &textPrinter{}
 }
 
-func NewTextPrinter(opts *TextOpts) (*TextPrinter, error) {
-	printer := &TextPrinter{}
-
-	return printer, nil
-}
-
-func (c *TextPrinter) Print(results []judge.Result) error {
+func (c *textPrinter) Print(results []judge.Result) error {
 
 	sort.Slice(results, func(i, j int) bool {
 		return results[i].Name < results[j].Name


Test patch:
diff --git a/cmd/kubent/main_test.go b/cmd/kubent/main_test.go
index 98b5419..b4bcb4d 100644
--- a/cmd/kubent/main_test.go
+++ b/cmd/kubent/main_test.go
@@ -20,7 +20,7 @@ func TestInitCollectors(t *testing.T) {
 		Helm3:      false,
 		Kubeconfig: "test",
 		LogLevel:   config.ZeroLogLevel(zerolog.ErrorLevel),
-		Output:     "test",
+		Output:     "text",
 	}
 
 	collectors := initCollectors(&testConfig)
diff --git a/pkg/printer/json_test.go b/pkg/printer/json_test.go
new file mode 100644
index 0000000..2867dbb
--- /dev/null
+++ b/pkg/printer/json_test.go
@@ -0,0 +1,47 @@
+package printer
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/doitintl/kube-no-trouble/pkg/judge"
+)
+
+var findingsJsonTesting []judge.Result = []judge.Result{
+	{
+		Name:        "testName1",
+		Kind:        "testKind1",
+		Namespace:   "testNamespace1",
+		ApiVersion:  "v1",
+		RuleSet:     "testRuleset1",
+		ReplaceWith: "testReplaceWith1",
+		Since:       "testSince1",
+	},
+	{
+		Name:        "testName2",
+		Kind:        "testKind2",
+		Namespace:   "testNamespace2",
+		ApiVersion:  "v1",
+		RuleSet:     "testRuleset2",
+		ReplaceWith: "testReplaceWith2",
+		Since:       "testSince2",
+	},
+}
+
+func TestJsonPopulateOutput(t *testing.T) {
+	printer := &jsonPrinter{}
+	output, err := printer.populateOutput(findingsJsonTesting)
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	var j []judge.Result
+	err = json.Unmarshal([]byte(output), &j)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(j) != 2 {
+		t.Error("wrong number of results")
+	}
+}
diff --git a/pkg/printer/printer_test.go b/pkg/printer/printer_test.go
new file mode 100644
index 0000000..28fff15
--- /dev/null
+++ b/pkg/printer/printer_test.go
@@ -0,0 +1,24 @@
+package printer
+
+import (
+	"testing"
+)
+
+func TestParsePrinters(t *testing.T) {
+	for k, v := range printers {
+		printer, err := NewPrinter(k)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if printer != v() {
+			t.Fatal("Should be a valid printer")
+		}
+	}
+}
+
+func TestInvalidStringForParsePrinters(t *testing.T) {
+	_, err := NewPrinter("BAD")
+	if err == nil {
+		t.Fatal(err)
+	}
+}